(Bug): Qt5: Knob background color doesn't change color when setting keyframe or expression

devernay commented 1 year ago

Make sure to follow our issue report guidelines

[X] I'm using the latest version of Natron (not required but recommended)
[X] I've restarted Natron and the issue persists
[X] I've run Natron via the command line and the issue persists
[X] I've followed the contributing guidelines to the best of my understanding
[X] My issue is not on the issue tracker or in a pull request already (go search for it and dig around a little bit!)
[X] This bug is reproducible

Natron version

2.5

Operating system

macOS Monterey / Qt5

System specs

No response

Did you install Natron using the official installer?

[ ] Yes, I used the official installer
[ ] No, I installed from a binary archive
[X] No, I compiled Natron from sources
[ ] No, I installed Natron via another method

Custom installation path

No response

What were you trying to do?

set a keyframe or expression on a knob.

What did you expect to happen? What happened instead?

Keyframe should change the knob background to blue, expression to green

Qt4 version works fine

Step-by-step reproduction instructions

create any node (eg blur)

set a keyframe or expression on any parameter

Additional details

No response

devernay commented 1 year ago

@YakoYakoYokuYoku can you take a look?

devernay commented 1 year ago

Note that it works fine on pulldowns and checkboxes:

YakoYakoYokuYoku commented 1 year ago

Note that it works fine on pulldowns and checkboxes

Worth to highlight that both ComboBox and AnimatedCheckBox use paintEvent to set their colors and not the mainstyle.qss styling sheet. I'll have to see a way to set the color values properly in the QSS file.

YakoYakoYokuYoku commented 1 year ago

Found it. Because Qt style sheets use the class name of a widget to apply CSS properties to it and Qt5 metaobjects store the class name with the namespace, then there's no way to select the widgets without their namespaces. The official Qt documentation recommends to prepend the namespace with -- instead of :: (e.g. Natron--SpinBox), Qt4 supports this kind of selections too. To fix this we have to prepend Natron-- to the corresponding patterns in mainstyle.qss or remove the Natron namespace from the C++ sources, the former being the easier but less flexible solution.

devernay commented 1 year ago

Nice find! I guess this means we'll have to make a Qt5-specific mainstyle.css (mainstyle-qt5.qss?). It will also break users' stylesheets, but if they installed one they should be able to edit it. I want to push Natron 2.5 as fast as possible (it's been waiting since July) so that we can start testing the Qt5 version and working on the build scripts (which represents a large amount of work). Most users rely on our binaries to use Natron.

Natron 2.5 is ready for every platform except WIndows @rodlie https://github.com/NatronGitHub/Natron/issues/828

YakoYakoYokuYoku commented 1 year ago

Nice find! I guess this means we'll have to make a Qt5-specific mainstyle.css (mainstyle-qt5.qss?). It will also break users' stylesheets, but if they installed one they should be able to edit it.

I was thinking for other options, one is to edit the CSS at load time which could be tricky but doable, another is to use both an application style and a color scheme like what Krita and many other Qt/KDE apps do.

I want to push Natron 2.5 as fast as possible (it's been waiting since July) so that we can start testing the Qt5 version and working on the build scripts (which represents a large amount of work). Most users rely on our binaries to use Natron.

Natron 2.5 is ready for every platform except WIndows @rodlie #828

I've been thinking, couldn't have been better to do a release candidate? :thinking:

devernay commented 1 year ago

I've been thinking, couldn't have been better to do a release candidate? 🤔

We (people doing the actual builds) don't have time for that, and we don't have enough testers anyway (two 2.5.0 alpha versions published, no feedback). Since there is almost 100% compatibility with 2.4, and it's easy to rollback, I don't think it's necessary.

devernay commented 1 year ago

And btw v2.5.0 is marked as a pre-release, and people can already download the builds. We may end up starting with a 2.5.1 release if anything is wrong with these.

devernay commented 1 year ago

fixed by https://github.com/NatronGitHub/Natron/pull/873

NatronGitHub / Natron