Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

full screen mode for main and bottom areas #4744

Merged
merged 1 commit into from
May 28, 2019
Merged

full screen mode for main and bottom areas #4744

merged 1 commit into from
May 28, 2019

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented Mar 28, 2019

fix #4668

TODO:

@AlexTugarev
Copy link
Contributor

@akosyakov, I really like this feature! one thing though: I'd prefer to call it maximized mode instead of full screen mode, which in fact would be:

if (document.fullscreenEnabled) {
  document.body.requestFullscreen()
}

@akosyakov
Copy link
Member Author

@AlexTugarev understood, how would you call a command Toggle Maximized?

@akosyakov
Copy link
Member Author

It's simple and naive implementation that one can maximize the terminal and see the whole output easier, does not work nicely for all cases. Please try if you find it nice we can start with it.

@akosyakov akosyakov changed the title WIP full screen mode for main and bottom areas full screen mode for main and bottom areas Mar 29, 2019
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maximizing editors in the main area works fine for me, there are some corner cases that don't work the best but they can always be improved iteratively.

Some cases do not work correctly for me for instance:

  1. triggering a new terminal and maximizing it means I can no longer see the output or write in it (might be the xterm issue).
  2. Complex main areas (two editors split with a widget) does not always recover well when minimizing. (this is likely a corner case so no need to spend too much time on it)
  3. I wonder if it is possible if you maximize a single editor in the main area (with a split present) it will take the full size of the screen and not preserve the split.

@akosyakov
Copy link
Member Author

triggering a new terminal and maximizing it means I can no longer see the output or write in it (might be the xterm issue).
Complex main areas (two editors split with a widget) does not always recover well when minimizing. (this is likely a corner case so no need to spend too much time on it)

These two i've tested and expect to work. If it does not work then it does not make sense. Which browser do you use?

I wonder if it is possible if you maximize a single editor in the main area (with a split present) it will take the full size of the screen and not preserve the split.

It is not easy. They get constantly resized by corresponding area. We would need to refactor shell to allow it. This PR does on level of css and it works nicely only for top-level areas.

@vince-fugnitto
Copy link
Member

triggering a new terminal and maximizing it means I can no longer see the output or write in it (might be the xterm issue).
Complex main areas (two editors split with a widget) does not always recover well when minimizing. (this is likely a corner case so no need to spend too much time on it)

These two i've tested and expect to work. If it does not work then it does not make sense. Which browser do you use?

I am running the PR on Gitpod and Chrome :(

terminal

@kittaakos
Copy link
Contributor

kittaakos commented Mar 29, 2019

  • exit on ESC - hard: it's colliding with monaco esc handlerd

Ctrl+M is the binding for Maximize Active View or Editor in Eclipse.
(I am using macOS, but if I remember correctly, the keybinding was the same on Windows too.)

Edit: fixed the command name taken from Eclipse.

@akosyakov akosyakov force-pushed the GH-4668 branch 3 times, most recently from aa95ab3 to a55f6d6 Compare March 31, 2019 03:09
@akosyakov
Copy link
Member Author

I had forgotten to push on Friday, so you tested the old state 😬 Please have a look again.

@kittaakos good idea! I went with Eclipse shortcut.

@vince-fugnitto I can reproduce an issue with xterm. It is not related to this PR, but general issue which should be fixed by #4680

With multi editors it works nicely for me:
maximized

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works great !
The only issue I had was with the terminal but as we both mentioned
it is likely due to xterm therefore I won't hold back this PR :)

@akosyakov
Copy link
Member Author

I will await merging it before issues with xtermjs are sorted out.

@@ -565,6 +580,10 @@ export class CommonFrontendContribution implements FrontendApplicationContributi
command: CommonCommands.COLLAPSE_ALL_PANELS.id,
keybinding: 'alt+shift+c',
},
{
command: CommonCommands.TOGGLE_MAXIMIZED.id,
keybinding: 'ctrlcmd+m',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: it cannot be ctrlcmd. Only ctrl is valid on OS X. Otherwise, you overrule macOS's minimize binding.

It is up to you what you pick, but I wrote Ctrl+M and you interpreted my comment as CtrlCmd+M

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not interpret 😄 I've tried to use Eclipse key-shortcut. I wonder how Eclipse deals with it. Don't it also overrule Mac OS binding, if os then it should be fine for us as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how Eclipse deals with it.

It is ctrl+m in Eclipse on macOS and not cmd+m.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right! just checked it, will change

had to trust to internet less and check with latest Eclipse first :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched to ctrl+m, unfortunately terminals have already bindings for it, so it does not work for them

@akosyakov
Copy link
Member Author

@kittaakos @vince-fugnitto @AlexTugarev xterm upgrade was merged. I've upgraded this PR to fix issues with unresponsive terminal after toggling maximization. I'm aways till the end of next week if you like it please merge yourself.

@kittaakos
Copy link
Contributor

It looks great, unfortunately, toggling the maximized state does not work reliably with the keybinding. It is broken in the terminal, and it does not seem to work in the preview editor either.

screencast 2019-05-16 08-36-29

it does not seem to work in the preview editor

I do not know how I managed to get the preview editor. But that's a separate issue presumably.

@kittaakos
Copy link
Contributor

It is broken in the terminal

Most likely the same as #5152 (comment).

@akosyakov
Copy link
Member Author

@kittaakos Would it be fine to address keybindings issue separately?

@kittaakos
Copy link
Contributor

@kittaakos Would it be fine to address keybindings issue separately?

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

double click on widget title to bring it in full window mode
4 participants