search

poush / H2

Supporting light weight extensions for heavy task lifting
MIT License
16 stars 32 forks source link

Add keyboard shortcut for fullscreen #26

Closed ghost closed 5 years ago

ghost commented 5 years ago

==== Tracking issue: #23

poush commented 5 years ago

Thanks for the PR, please have a look at some others things, I mentioned, that will be needed to ensure a stable and working full screen.

ghost commented 5 years ago

@poush Thanks for feedback! I'm new to electron so the extra guidance is appreciated. I'll take a look and make adjustments

poush commented 5 years ago

Sure, feel free to ask me anything and take your time. Here is some explanation of above.

  1. We have to enable fullscreen only when the keys are pressed. For this you can put this line mainWindow.setFullScreenable(true); inside the function called on keys event. This will do that.
  2. The frame is not showing when full screen in mac. If you are using different OS then please do check for that in your OS as well.
  3. We have to listen for "Esc" key only when the app is in full screen mode. (or if it is in focus, either will work)
  4. Lastly, there is an event fired when app exits the full screen so you have to listen for that event which is similar to listening will-quit event. In that event you have to make mainWindow.setFullScreenable(false); false again and if required remove any frame config (based on 2 )

If frame part seems confusing (or any part), then feel free to leave it and create an issue for it.

ghost commented 5 years ago

@poush I'm running into an interesting situation that I wanted to get your opinion on. I'm trying to leverage the exit-full-screen event to do the cleanup of turning off isFullScreenable but when I start the application up I am getting 6 instances of the event being emmited on a fresh open, not just when exiting full screen.

I've pushed my latest code to the feature branch. It may potentially be easier as well as cleaner if I just take the logic currently inlined where the handler for Alt+Shift+F is registered and extract to a module that accepts a reference to main window, so that way no code is repeated and the same cleanup would happen when exiting via Esc (not currently wired up) or by doing Alt+F+Shift a second time. What do you think?

Lastly, can you elaborate on what you mean by the frame?

Thanks again for taking the time to work through this with me!

poush commented 5 years ago

@jodylecompte I tried on my system but the 'leave-full-screen' is called only once and only when I actually exited the full screen. If you are saying exactly, 'exit-full-screen', then I couldn't find it on docs and it doesn't work either when I tried. Agree on the second part as we won't have to write again for two different key combinations. Interestingly, I get to experience one more thing. I made it full screen first then changed to a different screen and called the key combinations again to resize back to normal but it was gone and I couldn't see it anywhere and even clicking on tray icon couldn't focus and show it to me. So I guess, this will be another thing we will have to take care of. Please do make an issue of it once this is merged. Also, in case I press the combinations being on the same screen of H2 then it works.

The frame here is nothing but the window frame that encloses any window (or BrowserWindow in this case) which has basically the controls to exit, hide, maximize the app (on left in mac, right on win). The problem, it doesn't show when it is on full screen in macOS. Normally if you use Chrome in full screen, you can mouse over at the top of the display to see the frame coming down.

ghost commented 5 years ago

@poush The 'exit-full-screen' was a typo in my comment.

In breaking out the full screen logic into it's own function, I was able to do away with the event and just do the cleanup on exiting full screen -- in this case just setting allowFullscreenable back to false.

Regarding the disappearing window bug you mentioned, I am not able to reproduce on Windows10 or Ubuntu 18.04; based on the screenshot on the readme, I assume you're running a mac?

I'm also not able to find anything tangible in regards to figuring out how to enable the behavior of the frame, but I can confirm that it's also not working on the two systems above. I regretably need to tap out on those two issues, atleast for now until I get a little more comfortable with Electron.

So that considered, once this merged I need to create the following issues: 1) Disappearing Screen from full screen (Possible mac only bug) 2) Adding frame to control window with cursor in fullscreen mode

Does that sound / look good?

poush commented 5 years ago

@jodylecompte sounds perfect.

poush commented 5 years ago

@jodylecompte Please do also create an issue for "adding shortcuts.MD file in guides directory with the list of all keyboard shortcuts" once this is merged.

poush commented 5 years ago

This pull request introduces 1 alert when merging ee4134ab74c8d5a4f74126835715754cdc0589e5 into f4f66f88af820a61b79bdf2166b72cbabda6043c - view on LGTM.com

new alerts:

Comment posted by LGTM.com

ghost commented 5 years ago

The above error was introduced by correcting the merge conflict in the github UI. Will fix in subsequent commit

ghost commented 5 years ago

@poush I've got the shortcut on the task menu, but I am having a little bit of trouble getting the keyboard shortcut to work. I'm starting to wonder if it's even possible to do so, atleast on Ubuntu.

I pushed my current code, as well as resolved the new merge conflict, so you could take a look. What's curious is that if I take the same .createFromTemplate() call and produce a browser window menu instead, the shortcut works as intended with no modifications needed to the code; but does not seem to work for the tray menu.

I posted on StackOverflow after I could not find a hard yay or nay for shortcuts on a task context menu, but no answer yet. Do you have any ideas?

poush commented 5 years ago

Sorry, I not sure how I missed your last message. I have merged this and pushed a simple fix by using the rendering process for the local shortcut for now. It seems like disabling main menu removes all watch events for accelerators.

ghost commented 5 years ago

@poush Thanks for finding a solution, and for being so accomodating and helpful to new contributors!

It looks like the shortcuts issue you requested was already resolved by PR #61, so I just created the issues pertaining to the disappearing issue you observed, and for the frame configuration to allow exiting fullscreen with a cursor.