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.83k stars 339 forks source link

QMessageBoxes cause the application to segfault when QT_QPA_PLATFORMTHEME is set #346

Closed zjeffer closed 2 years ago

zjeffer commented 2 years ago

I'm using bspwm on Arch Linux. Whenever any QMessageBox is created, it will segfault. The first time it crashed, was in the Updater::setUpdateAvailable method from the QSimpleUpdater repo, which runs when the application starts. I altered the code so it wouldn't create a messagebox, and then it crashed here.

I installed notes from the aur and also tried building it myself. Both methods crash, but when I try the AppImage, everything works fine. This might be unrelated, but I did notice that the window has the macOS buttons, when it shouldn't display buttons at all on a tiling window manager like bspwm:

image

GDB backtrace: gdb.txt

nuttyartist commented 2 years ago

Thanks for letting us know! Do you think you know why the Appimage didn't produce the segfault but from the aur it did? BTW, I'm not maintaining the package from the aur.

Regarding the macOS buttons, you can always go to View-> show native window to have a native window decoration. Let me know if it works for you.

zjeffer commented 2 years ago

Do you think you know why the Appimage didn't produce the segfault but from the aur it did? BTW, I'm not maintaining the package from the aur.

Not yet, I will investigate.

Regarding the macOS buttons, you can always go to View-> show native window to have a native window decoration. Let me know if it works for you.

That works, thanks. But shouldn't native window decoration be the default for every desktop environment and window manager? What's the reason for this choice?

nuttyartist commented 2 years ago

But shouldn't native window decoration be the default for every desktop environment and window manager? What's the reason for this choice?

The reason is that our custom decoration will often look better than the native window decoration (mainly among different Linux distros), and aesthetics are very important to me. But I know it's not perfect. This is why the native window option. I also aspire to have custom os-specific code in each operating system to have a native experience (somewhat successfully in macOS). Hopefully, we can improve the native experience in Windows as well.

zjeffer commented 2 years ago

I wanted to build Qt 5.15.6 from source, to be able to see inside Qt's source code when debugging in Qt Creator, but the issue doesn't happen when I point my Qt installation to the one built from source :(

The Qt version is the exact same as the one that was already installed on my system. I did a reinstall of Qt5 through my package manager, but nothing helped. Very strange.

zjeffer commented 2 years ago

Running notes as sudo doesn't trigger the crash. Any ideas why it wouldn't?

nuttyartist commented 2 years ago

Very weird indeed. I have no idea yet.

zjeffer commented 2 years ago

FOUND IT!

The cause of the problem is the value I gave to the QT_QPA_PLATFORMTHEME environment variable in my ~/.xinitrc file.

The env variable is normally set to qt5ct, but removing the value fixes it:

image

Does notes do anything special with the Qt's platformtheme plugin?

zjeffer commented 2 years ago

If anyone is interested, here's how I managed to find this: I ran lsof -p $(pidof notes) once when running notes normally, and once with the Qt I built from source (the one that doesn't crash). I compared the two outputs and found the one that crashed contained /usr/lib/qt/plugins/platformthemes/libqt5ct.so, while the one that worked didn't.

zjeffer commented 2 years ago

Setting that env variable to qt6ct also fixes the problem.

This is my ~/.config/qt5ct/qt5ct.conf:

[Appearance]
color_scheme_path=/usr/share/qt5ct/colors/airy.conf
custom_palette=false
icon_theme=Papirus-Light-nordic-blue-folders
standard_dialogs=default
style=kvantum

[Fonts]
fixed=@Variant(\0\0\0@\0\0\0\x1e\0\x44\0\x65\0j\0\x61\0V\0u\0 \0L\0G\0\x43\0 \0S\0\x61\0n\0s@(\0\0\0\0\0\0\xff\xff\xff\xff\x5\x1\0\x32\x10)
general=@Variant(\0\0\0@\0\0\0\x1e\0\x44\0\x65\0j\0\x61\0V\0u\0 \0L\0G\0\x43\0 \0S\0\x61\0n\0s@(\0\0\0\0\0\0\xff\xff\xff\xff\x5\x1\0\x32\x10)

[Interface]
activate_item_on_single_click=1
buttonbox_layout=0
cursor_flash_time=1000
dialog_buttons_have_icons=1
double_click_interval=400
gui_effects=@Invalid()
keyboard_scheme=2
menus_have_icons=true
show_shortcuts_in_context_menus=true
stylesheets=@Invalid()
toolbutton_style=4
underline_shortcut=1
wheel_scroll_lines=3

[SettingsWindow]
geometry=@ByteArray(\x1\xd9\xd0\xcb\0\x3\0\0\0\0\x13\x45\0\0\0\xd0\0\0\x16\xec\0\0\x3\xaf\0\0\x13H\0\0\0\xd3\0\0\x16\xe9\0\0\x3\xac\0\0\0\x2\0\0\0\0\a\x80\0\0\x13H\0\0\0\xd3\0\0\x16\xe9\0\0\x3\xac)

[Troubleshooting]
force_raster_widgets=1
ignored_applications=@Invalid()

Setting style= to Fusion (the default) also fixes the issue (but makes Qt apps uglier). Something strange is happening when it is set to kvantum...

nuttyartist commented 2 years ago

So is it happening only on a specific distro? Do you have a suggestion on how we can solve this when shipping our app?

zjeffer commented 2 years ago

I believe this will happen on any distro when QT_QPA_PLATFORMTHEME is set to qt5ct, when the theme in qt5ct.conf is set to kvantum.

As for how we can solve this, I'm still investigating why this happens in the first place. I haven't gotten this issue in any other QT app, so there must be something unique about your app that's causing this.

nuttyartist commented 2 years ago

Hmm, ok, I hope you'll find out what causes this.

zjeffer commented 2 years ago

The issue is fixed when removing this line: https://github.com/nuttyartist/notes/blob/dev/src/main.cpp#L14.

Why is this set to false? Setting it to true (the default) has no impact on my system.

bjorn commented 2 years ago

That's an interesting find. There's still another thing actually causing the bug, because this setting, whether true or false should not be causing the application to crash.

The line was added in c5d0e95a7972b1de2be66e1f6674d96a334df188, and its description implies it's only for the style, but these days the style is already sneakily set by the TreeViewLogic:

https://github.com/nuttyartist/notes/blob/d1d3a4518c687dc51935b638d7a7756b41279a4a/src/treeviewlogic.cpp#L88-L89

So personally I think we should remove this line. Better let the application be desktop settings aware for everything, and then override these kind of settings as needed. You never know what is covered by this vague description "the system's standard colors, fonts, etc." though, so when we remove the line, somebody (probably @nuttyartist) needs to check on each platform for potentially undesired color, font, etc. changes.

(well, I see the CustomApplicationStyle is just a proxy style, but anyway, we need to check for changes on all platforms after removing this...)

zjeffer commented 2 years ago

I agree. I believe the issue is that this app's custom style does not include styling for DialogButtonBox (which is present in QMessageBox), which is why it segfaults when it tries to find a style. The app can override the system's theme when it includes styling for an element, but use the system's theme when it doesn't.

nuttyartist commented 2 years ago

I agree. We can remove this. I will check how everything looks before merging.

Good find @zjeffer.

Someone sends a PR?