nuttyartist / notes

Fast and beautiful note-taking app written in C++. Write down your thoughts.
https://notes-foss.com
Mozilla Public License 2.0
3.72k stars 326 forks source link

Only enable 'stay on top' feature on macOS and Windows #491

Closed guihkx closed 1 year ago

guihkx commented 1 year ago

On other platforms (e.g. Linux, BSDs), we remove the code entirely.

bjorn commented 1 year ago

What is the rationale behind this? Because generally on Linux, the window manager already provides this feature? Or is there an advantage of not providing this option beyond that?

guihkx commented 1 year ago

This feature was disabled on Linux by ccf71e5760660449e8a4669d154300a6a6355083.

All this PR does is just remove the code from some other places.

For example, Notes currently calls toggleStayOnTop() when you press Ctrl+K:

https://github.com/nuttyartist/notes/blob/1e2bab885da98137ab4f9fa312e51e25525a7ebc/src/mainwindow.cpp#L477

Even though this feature works well on KDE Plasma 5.27, on GNOME 43 it doesn't, for some reason.

So, in my opinion, it should be okay to keep this feature removed from Linux/BSDs altogether, because like you said, most window managers already offer this natively, including KWin.

nuttyartist commented 1 year ago

So, in my opinion, it should be okay to keep this feature removed from Linux/BSDs altogether, because like you said, most window managers already offer this natively, including KWin.

But how will people call this action if it's not offered via our app? Some system-wide shortcut? A good UX is to allow users to discover the different options available (usually via a menu).

guihkx commented 1 year ago

But how will people call this action if it's not offered via our app?

Windows and macOS users will still be able to use this feature normally (if that's what you're asking).

A good UX is to allow users to discover the different options available (usually via a menu).

This option was not showing up on Linux anyway, that's why I'm completely removing it from other places (for Linux and BSDs only, macOS is not included).

Here's a screenshot from the official v2.0.0 AppImage (notice how the option to make the window stay on top is not even there):

image

And here's a screenshot from the official v2.0.0 Windows build, where the option is present:

image

nuttyartist commented 1 year ago

Oh, ok. Forgot about that hah. So why not add this option to Linux users?

guihkx commented 1 year ago

So why not add this option to Linux users?

Even though this feature works well on KDE Plasma 5.27, on GNOME 43 it doesn't, for some reason.

It's definitely possible that this is a GNOME-only issue (I have not tested with other desktop environments), but like I said, on Linux it's common for window managers to implement this feature natively, so in my opinion, it should be okay to remove it completely.

bjorn commented 1 year ago

This feature was disabled on Linux by ccf71e5.

LOL, my own change. Totally forgot that this feature wasn't working on GNOME anyway. :-)

bjorn commented 1 year ago

I approved this, but I do think this change adds more maintenance overhead than necessary for disabling this feature on Linux. I think it would suffice for example, to just disable the line registering the shortcut, since there is no harm in keeping the rest of the code, and that way it's easier to re-enable in case we later wanted to try making it work.

nuttyartist commented 1 year ago

I'm with @bjorn on this.

guihkx commented 1 year ago

No problem, I'll just disable the keyboard shortcut on Linux then. :)