search

fstl-app / fstl

A fast STL file viewer
472 stars 105 forks source link

Implement compact and fullscreen view #81

Closed sur5r closed 2 years ago

sur5r commented 2 years ago

This implements both a compact view (hiding the menu bar) and a fullscreen mode.

Fixes https://github.com/fstl-app/fstl/issues/80

mboerwinkle commented 2 years ago

Nice work! I have a few suggestions:

  1. Entering and leaving fullscreen should not change compact-mode state. I like being in fullscreen and having a menu bar. Also, the ability to return from compact mode will still exist by restarting the application.
  2. Fullscreen should present a checkbox in its menu item (the same way compact mode does).

If you do not want to decouple fullscreen and compact, there is a small issue with the existing fullscreen/compact link. Some window managers allow you to cause a window to enter fullscreen, so entering/leaving compact mode needs to be a result of the 'entering/leaving fullscreen' event, not the button press (since fullscreen events will not always be a result of the menu button/f11).

Also, I would have named 'Compact Mode' as 'Hide Menu Bar', but this is not a strong preference; please take no offense.

I am happy to submit patches for any of the above that you have interest in - please let me know if so.

Finally, I would like 'Compact Mode' to be persisted when restarting the application, but I don't know a good way for users to exit it in that case without knowing the key shortcut (or knowing that entering and leaving fullscreen may trigger it). Maybe this is something command line arguments can help with, but I think that is out of the scope of this patch :/

Best, Martin

sur5r commented 2 years ago

Thanks for the feedback!

The window manager interaction is something I thought about as well, but only after opening this PR. I guess I got carried away while playing with the code.

Decoupling both functions does make sense, yes.

I'm not sure whether it should persist. But this can indeed be implemented separately.

I will force-push a new implementation with the following changes: