Added ability to properly remove icons from menus (for a cleaner UX) #3230

andrei-a-papou · 2025-02-10T11:06:25Z

Hi, and thank you for creating QOwnNotes. I've been using it for a couple of weeks now (migrated from a legacy note-taking app) and it looks very impressive so far! Packed with features, very customizable and scriptable.

I'm using it on a mostly GTK-based Linux desktop where menus do not have icons and would like to have the same option in QOwnNotes for a cleaner user experience.

I'm very new to Qt and it took some research to properly remove the icons.

First, I've tried to do it the way macOSX icons are removed in the QOwnNotes code-base (Qt::AA_DontShowIconsInMenus), but this method leaves empty spaces in place of the icons (at least on Linux), which doesn't look good at all, IMO.

Another option is to simply iterate over all the actions and set icons to null. This removes the icons and the empty space, but kill icons in toolbars too.

After experimenting, I've found that the best way is to use QProxyStyle. Hence this patch.

I'm not interested in maintaining my own fork :), so would appreciate it if you could merge it on your end. I've noticed some other little bugs/quirks here and there. I'll try to report them soon as time permits.

Overall, QOwnNotes looks like a great app with a ton of useful features. So thank you again for creating it and let me know if the patch can be merged.

src/mainwindow.h

pbek · 2025-02-10T12:02:26Z

There already is a rule for macOS 🤔:

QOwnNotes/src/mainwindow.cpp

Lines 152 to 155 in 0d11bd1

    
           #ifdef Q_OS_MAC 
        
               // disable icons in the menu 
        
               QApplication::instance()->setAttribute(Qt::AA_DontShowIconsInMenus, true); 
        
           #endif

We need to do something about that too...

andrei-a-papou · 2025-02-10T12:11:00Z

There already is a rule for macOS 🤔:

I don't have a way to test things on a Mac, but would it be OK to limit the patch to Linux and Windows via #ifdef and such? The existing Mac logic can stay the way it is. Or it can be removed (but unfortunately I wouldn't be able to test the result on a Mac.)

So my suggestion would be to limit the patch to Windows and Linux only. Your thoughts?

pbek · 2025-02-10T13:32:42Z

First one other question: Why was QApplication::instance()->setAttribute(Qt::AA_DontShowIconsInMenus, true); not enough for your use-case?

Seems to work fine under Linux. The command bar is extra of course.

I guess it would be best to stick to QApplication::instance()->setAttribute(Qt::AA_DontShowIconsInMenus, true); and just set the default for your setting to be true for macOS by default. Of course, the settings reading needs to go into a method then, like so:

QOwnNotes/src/utils/misc.cpp

Lines 1299 to 1301 in a1c7a31

    
           bool Utils::Misc::isNoteEditingAllowed() { 
        
               return SettingsService().value(QStringLiteral("allowNoteEditing"), true).toBool(); 
        
           }

src/dialogs/settingsdialog.ui

andrei-a-papou · 2025-02-10T15:31:25Z

First one other question: Why was QApplication::instance()->setAttribute(Qt::AA_DontShowIconsInMenus, true); not enough for your use-case?

No, not enough, that's the whole point. The Qt::AA_DontShowIconsInMenus removes the icons, but leaves a lot of ugly empty space where the icons used to be. IMO, this makes the menus look worse, not better, especially if the menu contains checkboxes.

Not only do the empty spaces look bad, they also make the menus with hidden icons look different from other menus that are icon-less by design, for example, the Toolbars menu. This inconsistency IMO would make the UI look less professional than it could be :)

pbek · 2025-02-10T19:17:16Z

just set the default for your setting to be true for macOS by default. Of course, the settings reading needs to go into a method then

ok, then still ☝🏻 and the builds are still failing, because of the duplicate declaration (how did the build work for you? 🤔)

andrei-a-papou · 2025-02-10T19:24:12Z

just set the default for your setting to be true for macOS by default. Of course, the settings reading needs to go into a method then

ok, then still ☝🏻 and the builds are still failing, because of the duplicate declaration (how did the build work for you?

You're right. The build worked for me because the patch I provided depends on another patch (not included here for now).

Fixed it here. It now compiles on my end when I do a clean qmake && make from the github source tree.

andrei-a-papou · 2025-02-10T19:33:30Z

@pbek Some builds failed due to undefined reference to `vtable for NoMenuIconStyle', which is fixed now.

pbek · 2025-02-10T19:43:24Z

Please use lower case characters for NoMenuIconStyle.* and move the files to the src/helpers folder.
Please also add the two files to src/CMakeLists.txt.
And please run ./scripts/clang-format-project.sh on your changes...

pbek · 2025-02-10T19:46:42Z

src/utils/misc.cpp

+ * @return
+ */
+bool Utils::Misc::hideIconsInMenus() {
+    return SettingsService().value(QStringLiteral("hideIconsInMenus"), true).toBool();


Default needs to be true for macOS and false for all other operating systems. 😉

Yes, I'm now setting the default here: 259c49f

You could instead of setting true just use a variable that is set to false and set it to true with a compiler directive for macOS.

I'm sorry, I don't quite follow... Do you mean we shouldn't use if (!settings.contains(QStringLiteral("hideIconsInMenus"))) { ... }? if so, why?

I'm new to Qt and the code-base, so if you think there's a better way to do the macOS check, please feel free to give it a quick edit :)

See #3230 (comment)

src/utils/misc.cpp

pbek · 2025-02-10T19:52:09Z

src/main.cpp

+    }
+
+#ifdef Q_OS_MAC
+    QApplication::setStyle(new NoMenuIconStyle);


Rather than forcing the style for macOS, please use your new setting instead and set a proper default value in your hideIconsInMenus method.

OK, no forcing, but can we just set a default like I did here? 259c49f

I've tested it under Linux with #ifdef Q_OS_LINUX instead of #ifdef Q_OS_MAC and it seems to work fine.

When no setting has been defined, the app hides icons on macOS. If a user ticks the checkbox, the app reads that setting instead.

I was talking about a variable for the default value, like here:

QOwnNotes/src/dialogs/settingsdialog.cpp

Lines 1058 to 1066 in c6c3589

#ifdef Q_OS_MAC

bool restoreCursorPositionDefault = false;

#else

bool restoreCursorPositionDefault = true;

#endif

ui->restoreCursorPositionCheckBox->setChecked(

settings.value(QStringLiteral("restoreCursorPosition"), restoreCursorPositionDefault)

.toBool());

OK, how about this? 7f7cc8c

andrei-a-papou · 2025-02-10T21:21:01Z

Please use lower case characters for NoMenuIconStyle.* and move the files to the src/helpers folder. Please also add the two files to src/CMakeLists.txt. And please run ./scripts/clang-format-project.sh on your changes...

Renamed and moved the files, but the clang-format-project.sh doesn't work on my machine. This is the output of clang-format --version:

clang-format --version
Debian clang-format version 11.0.1-2

Running the script produces a bunch of "not found" errors...

pbek · 2025-02-10T21:53:50Z

Debian clang-format version 11.0.1-2

omg, I'm, at clang-format version 19.1.5

andrei-a-papou · 2025-02-10T23:21:09Z

Debian clang-format version 11.0.1-2

omg, I'm, at clang-format version 19.1.5

Can you please run the script then?

pbek · 2025-02-11T12:28:13Z

src/utils/misc.cpp

+
+    SettingsService settings;
+
+    if (!settings.contains(QStringLiteral("hideIconsInMenus"))) {


You don't need that whole if-clause, because there is no old setting to migrate from.

src/utils/misc.cpp

pbek · 2025-02-11T19:02:28Z

Oh, there is something important that slipped my review:

QOwnNotes/src/main.cpp

Lines 192 to 201 in da690c2

    
           // restore the interface style 
        
           if (!interfaceStyle.isEmpty()) { 
        
               QApplication::setStyle(interfaceStyle); 
        
           } 
        
           // apply custom style to hide menu icons 
        
           const bool hideMenuIcons = Utils::Misc::areMenuIconsHidden(); 
        
           if (hideMenuIcons) { 
        
               QApplication::setStyle(new NoMenuIconStyle); 
        
           }

You are breaking the interface style setting with your QApplication::setStyle!

Signed-off-by: Patrizio Bekerle <patrizio@bekerle.com>

pbek · 2025-02-11T19:17:14Z

I'll disable your QApplication::setStyle(new NoMenuIconStyle); in favor for QApplication::setAttribute(Qt::AA_DontShowIconsInMenus, true); for now until you come up with a different idea.

But I still can't see a difference between the two in the menu. 🤔 Could you maybe provide screenshots where you see a difference on your system?

…erfaceStyle settting Signed-off-by: Patrizio Bekerle <patrizio@bekerle.com>

Signed-off-by: Patrizio Bekerle <patrizio@bekerle.com>

andrei-a-papou · 2025-02-11T20:22:31Z

I'll disable your QApplication::setStyle(new NoMenuIconStyle); in favor for QApplication::setAttribute(Qt::AA_DontShowIconsInMenus, true); for now until you come up with a different idea.

This is very frustrating, I'm sorry to say :) Because now it looks broken...

But I still can't see a difference between the two in the menu. 🤔 Could you maybe provide screenshots where you see a difference on your system?

Sure. The first screenshot uses Qt::AA_DontShowIconsInMenus.

The second screenshot uses the PR that actually deals with the problem:

pbek · 2025-02-11T21:01:55Z

No difference for under KDE Plasma...

QApplication::setAttribute(Qt::AA_DontShowIconsInMenus, true);

QApplication::setStyle(new NoMenuIconStyle);

pbek · 2025-02-11T21:04:29Z

Maybe you can find a solution, that will still keep the interfaceStyle setting.

andrei-a-papou · 2025-02-11T22:10:46Z

It's interesting that Qt::AA_DontShowIconsInMenus works under KDE Plasma. Maybe it's a Qt5 vs Qt6 thing, who knows, but I know for sure I'm not the only one having this problem: https://stackoverflow.com/questions/44745459/remove-icon-space-from-qmenu

Would you accept a patch that relies on NoMenuIconStyle, but enables the option to hide icons only if "Interface style" is set to Automatic? (macOS can use Qt::AA_DontShowIconsInMenus like before.)

pbek · 2025-02-12T05:35:04Z

Would you accept a patch that relies on NoMenuIconStyle, but enables the option to hide icons only if "Interface style" is set to Automatic?

That would be an option, but you need to keep in mind that the interfaceStyle setting is applied dynamically while the setting dialog is opened and when the selectbox in the dialog changes!

The better solution would be if your style could somehow inherit the interfaceStyle, maybe like in the example from https://doc.qt.io/qt-6/qapplication.html#setStyle.

(macOS can use Qt::AA_DontShowIconsInMenus like before.)

I guess macOS can use the same handling as above...

andrei-a-papou · 2025-02-12T12:33:11Z

The better solution would be if your style could somehow inherit the interfaceStyle

Thanks for the pointer. Right now time does not permit me to work on any more patches, but maybe in the future. Good luck!

pbek · 2025-02-12T13:18:51Z

Thank you, and thank you for your contribution!

pbek · 2025-02-12T20:02:36Z

25.2.4

There now is a setting to hide the icons in the main menu and command bar in the Interface settings
(for #3230, thank you, @andrei-a-papou)

There now is a new release, could you please test it and report if it works for you?

…erfaceStyle setting Signed-off-by: Patrizio Bekerle <patrizio@bekerle.com>

pbek · 2025-02-12T20:48:00Z

Can you please also test the latest main branch with 854a3ab, where I tried to make interfaceStyle work with NoMenuIconStyle?

Can you please test different interfaceStyle settings with hideIconsInMenus set to true and false?

Signed-off-by: Patrizio Bekerle <patrizio@bekerle.com>

andrei-a-papou · 2025-02-12T21:23:59Z

Can you please also test the latest main branch with 854a3ab, where I tried to make interfaceStyle work with NoMenuIconStyle?

Can you please test different interfaceStyle settings with hideIconsInMenus set to true and false?

The 854a3ab commit works fine, I tested with the "Automatic" setting for style (which happens to be Kvantum on my box), as well as "motif", "windows" and "plastique" styles. Awesome!

I've tried with "hideIconsInMenus" on and off.

There's one hiccup: with "hideIconsInMenus" on, if a user changes the interface style, the style is applied, but the menu icons reappear until the app is restarted. Seems like prompting for a restart after changing the interface style would make everything work perfectly.

Will be happy to test.

P. S. Incidentally, I now remember playing with interface styles when I first installed QOwnNotes, and I think the styles did not always perfectly applied unless I restarted the app. Seems like it's a good idea to prompt for a restart when changing a style anyway.

pbek · 2025-02-13T08:26:23Z

There's one hiccup: with "hideIconsInMenus" on, if a user changes the interface style, the style is applied, but the menu icons reappear until the app is restarted. Seems like prompting for a restart after changing the interface style would make everything work perfectly.

Hah, didn't catch my eyes. Thank you for mentioning. Forgot a stray QApplication::setStyle(). I'll fix that.

P. S. Incidentally, I now remember playing with interface styles when I first installed QOwnNotes, and I think the styles did not always perfectly applied unless I restarted the app. Seems like it's a good idea to prompt for a restart when changing a style anyway.

QApplication::setStyle() is supposed to work instantly.
But now they are applying instantly for you too?

andrei-a-papou · 2025-02-13T09:09:14Z

QApplication::setStyle() is supposed to work instantly.
But now they are applying instantly for you too?

Yes. Everything works as expected, except the hiccup I mentioned above.

pbek · 2025-02-13T10:21:39Z

Yes. Everything works as expected, except the hiccup I mentioned above.

Ok, great. I'll fix the hiccup in the evening.

Signed-off-by: Patrizio Bekerle <patrizio@bekerle.com>

pbek · 2025-02-13T19:37:14Z

25.2.5

The main menu and command bar now has a cleaner user experience when icons are hidden
on GTK-based Linux desktops (for #3230)

There now is a new release, could you please test it and report if it works for you?

andrei-a-papou · 2025-02-14T17:19:57Z

@pbek Works nicely now, thank you!

pbek · 2025-02-14T17:51:45Z

Great, thank you for testing!

Added ability to properly remove icons from menus (for a cleaner UX)

c188f99

pbek reviewed Feb 10, 2025

View reviewed changes

src/mainwindow.h Outdated Show resolved Hide resolved

pbek reviewed Feb 10, 2025

View reviewed changes

src/dialogs/settingsdialog.ui Outdated Show resolved Hide resolved

andrei-a-papou added 6 commits February 10, 2025 20:21

fixed redefinition

53c545d

use "hide" instead of "disable" wording

9ae2542

removed legacy macOS logic

b6ed5a0

do a class implementation in the CPP file

7fafda7

updated checkbox desc in UI

00e16e7

refactoring

08021a9

refactoring

28dc7f6

pbek reviewed Feb 10, 2025

View reviewed changes

src/utils/misc.cpp Outdated Show resolved Hide resolved

pbek reviewed Feb 10, 2025

View reviewed changes

andrei-a-papou added 6 commits February 10, 2025 23:40

reworked macOS logic

259c49f

renamed files

3fe840c

refactoring

82baa2d

renamed function

58458cf

refactoring

0056bba

fixes

767dcb8

reworked logic

7f7cc8c

pbek reviewed Feb 11, 2025

View reviewed changes

src/utils/misc.cpp Outdated Show resolved Hide resolved

more refactoring

748cdc3

pbek merged commit da690c2 into pbek:main Feb 11, 2025
32 of 33 checks passed

pbek added a commit that referenced this pull request Feb 11, 2025

lint #3230: format code using clang-format

3289322

Signed-off-by: Patrizio Bekerle <patrizio@bekerle.com>

pbek added a commit that referenced this pull request Feb 11, 2025

fix #3230: disable QApplication::setStyle, because it is breaking int…

7cf3413

…erfaceStyle settting Signed-off-by: Patrizio Bekerle <patrizio@bekerle.com>

pbek added a commit that referenced this pull request Feb 11, 2025

#3230 i18n: update translation file

d32fa50

Signed-off-by: Patrizio Bekerle <patrizio@bekerle.com>

pbek added a commit that referenced this pull request Feb 11, 2025

#3230 doc: update changelog

ec14ef3

Signed-off-by: Patrizio Bekerle <patrizio@bekerle.com>

pbek added a commit that referenced this pull request Feb 12, 2025

#3230 style: make hideIconsInMenus with NoMenuIconStyle work with int…

854a3ab

…erfaceStyle setting Signed-off-by: Patrizio Bekerle <patrizio@bekerle.com>

pbek added a commit that referenced this pull request Feb 12, 2025

#3230 test: add missing includes

5f6c91a

Signed-off-by: Patrizio Bekerle <patrizio@bekerle.com>

pbek added a commit that referenced this pull request Feb 13, 2025

#3230 style: fix hideIconsInMenus after switching styles

de322d2

Signed-off-by: Patrizio Bekerle <patrizio@bekerle.com>

	#ifdef Q_OS_MAC
	bool restoreCursorPositionDefault = false;
	#else
	bool restoreCursorPositionDefault = true;
	#endif

	ui->restoreCursorPositionCheckBox->setChecked(
	settings.value(QStringLiteral("restoreCursorPosition"), restoreCursorPositionDefault)
	.toBool());


		SettingsService settings;

		if (!settings.contains(QStringLiteral("hideIconsInMenus"))) {

Added ability to properly remove icons from menus (for a cleaner UX) #3230

Added ability to properly remove icons from menus (for a cleaner UX) #3230

Conversation

andrei-a-papou commented Feb 10, 2025

pbek commented Feb 10, 2025

andrei-a-papou commented Feb 10, 2025 • edited Loading

pbek commented Feb 10, 2025 • edited Loading

andrei-a-papou commented Feb 10, 2025 • edited Loading

pbek commented Feb 10, 2025 • edited Loading

andrei-a-papou commented Feb 10, 2025

andrei-a-papou commented Feb 10, 2025

pbek commented Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrei-a-papou Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrei-a-papou commented Feb 10, 2025

pbek commented Feb 10, 2025

andrei-a-papou commented Feb 10, 2025

Choose a reason for hiding this comment

pbek commented Feb 11, 2025

pbek commented Feb 11, 2025 • edited Loading

andrei-a-papou commented Feb 11, 2025

pbek commented Feb 11, 2025

QApplication::setAttribute(Qt::AA_DontShowIconsInMenus, true);

QApplication::setStyle(new NoMenuIconStyle);

pbek commented Feb 11, 2025

andrei-a-papou commented Feb 11, 2025

pbek commented Feb 12, 2025

andrei-a-papou commented Feb 12, 2025

pbek commented Feb 12, 2025

pbek commented Feb 12, 2025

25.2.4

pbek commented Feb 12, 2025

andrei-a-papou commented Feb 12, 2025 • edited Loading

pbek commented Feb 13, 2025

andrei-a-papou commented Feb 13, 2025

pbek commented Feb 13, 2025

pbek commented Feb 13, 2025

25.2.5

andrei-a-papou commented Feb 14, 2025

pbek commented Feb 14, 2025

andrei-a-papou commented Feb 10, 2025 •

edited

Loading

pbek commented Feb 10, 2025 •

edited

Loading

andrei-a-papou commented Feb 10, 2025 •

edited

Loading

pbek commented Feb 10, 2025 •

edited

Loading

pbek commented Feb 10, 2025 •

edited

Loading

andrei-a-papou Feb 10, 2025 •

edited

Loading

pbek commented Feb 11, 2025 •

edited

Loading

andrei-a-papou commented Feb 12, 2025 •

edited

Loading