lxqt / lxqt-qtplugin

LXQt Qt platform integration plugin
https://lxqt.github.io
GNU Lesser General Public License v2.1
24 stars 14 forks source link

Do not reset widget palettes on changing style #58

Closed tsujan closed 4 years ago

tsujan commented 4 years ago

Generally, resetting a widget's palette from outside its code is a bad practice because the palette may have been changed for a reason.

If the custom palette of a widget needs an update when the style changes, the program itself is responsible for that; otherwise, the program has a bug — as in libfm-qt's places view (which I'll fix soon) and Dolphin's main view (which will never be fixed).

tsujan commented 4 years ago

@yan12125 Unfortunately, I wasn't aware of the problem before 0.15.1. I don't know if it's serious or can wait until this patch goes into the next major release.

tsujan commented 4 years ago

The above-mentioned fix for libfm-qt is here: https://github.com/lxqt/libfm-qt/pull/545

yan12125 commented 4 years ago

Does this take the first approach mentioned in https://github.com/lxqt/lxqt-qtplugin/pull/56#issuecomment-636865071?

On the one hand, if we don't set the widget palettes to the app palette, the text colors of some widgets (like the side-pane of libfm-qt) won't be updated on switching between dark and light colors.


Unfortunately, I wasn't aware of the problem before 0.15.1. I don't know if it's serious or can wait until this patch goes into the next major release.

I don't consider a serious bug as changing palettes on the fly is a rare event for most users. For average users, it will not even happen before https://github.com/lxqt/lxqt-config/pull/629 is merged.

yan12125 commented 4 years ago

As a record, this also fixes an issue with qtermwidget - after changing window color on the fly, transparent background images become black.

tsujan commented 4 years ago

@yan12125 Setting widget palettes from outside the app code was a big mistake of mine, which is fixed here. Rule of thumb: Never set widget palette from outside the app code; set the app palette instead! Some of the reasons:

  1. A widget palette may be set by the app('s author) to customize the look of a widget. Overriding it would interfere with the app code.
  2. If the widget palette is set, Qt will set the attribute Qt::WA_SetPalette to true for it not to change again when the app palette changes. In other words, the widget won't inherit its palette from the app anymore — it'll become isolated.

For other reasons whose explanations are beyond this comment, the above-mentioned rule may be contradicted by Qt widget styles that have their own ways of drawing widgets — but only with the utmost care. Please see this explanation in Qt doc: https://doc.qt.io/qt-5/qapplication.html#setPalette → "Note: Some styles do not use the palette..." Fusion and Breeze aren't among such styles; that's why we can change their main colors with our plugin.

tsujan commented 4 years ago

I don't consider a serious bug as changing palettes on the fly is a rare event for most users. For average users, it will not even happen before lxqt/lxqt-config#629 is merged.

If the user changes other appearance settings, the palette will be applied and he/she might see some problems without the patch for lxqt-config too. However, they may not be terrible because they'll be fixed by restarting apps. I may have lost my perspective about what's serious and what's not; so, I'll do as you and other devs tell.

yan12125 commented 4 years ago

If the user changes other appearance settings, the palette will be applied and he/she might see some problems without the patch for lxqt-config too.

Sorry forget about that. But I still don't think it's a severe bug. Most users do not change system appearance often.

If another dev or user thinks a new release is needed for this bug, I'm fine with that, too.

tsujan commented 4 years ago

@yan12125 @luis-pereira Can this be merged? I want to add a simple workaround for the highlight color.

yan12125 commented 4 years ago

LGTM on my side. I thought I have approved this but it turns out my memory is wrong.

tsujan commented 4 years ago

I thought I have approved

It was my fault; forgot to request reviews.