Bug 357893 - Applications shouldn't override user-specified desktop settings (style)
Summary: Applications shouldn't override user-specified desktop settings (style)
Status: RESOLVED INTENTIONAL
Alias: None
Product: kdenlive
Classification: Applications
Component: User Interface (show other bugs)
Version: unspecified
Platform: Compiled Sources All
: NOR normal
Target Milestone: ---
Assignee: Jean-Baptiste Mardelle
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-12 17:53 UTC by RJVB
Modified: 2016-10-08 15:44 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
This seems a more reasonable implementation (2.78 KB, patch)
2016-01-12 18:01 UTC, RJVB
Details

Note You need to log in before you can comment on or make changes to this bug.
Description RJVB 2016-01-12 17:53:10 UTC
Somewhere after 15.12.0 release, kdenlive started imposing Breeze as the initial style unless the user configured something else via the Style menu.

That's wrongful behaviour: the style menu provides a courtesy/convenience feature to define a different style from what has been configured for the desktop. Under no circumstances should an application second-guess the user's preference in this kind of domain -- not if it otherwise has full support for the various alternatives that exist.

The "Default" entry in the Styles menu should go back to the user's default, not KDE's default. A application-preferred setting could optionally be provided as "Factory".

It seems this was introduced in commit #eb36d846 .

Reproducible: Always

Steps to Reproduce:
1. deconfigure any kdenlive-specific setting for "Style"
2. install kdenlive git/master
2. start it


Actual Results:  
Kdenlive starts up in Breeze if that style is installed.

Expected Results:  
Kdenlive should start up using the user's style of choice.

NB:
The check (`if (!desktopStyle.isEmpty() && QString::compare(desktopStyle, QLatin1String("breeze") ...`) actually takes no action if for some reason `desktopStyle` remained empty, or if Breeze isn't installed.
Comment 1 RJVB 2016-01-12 18:01:26 UTC
Created attachment 96605 [details]
This seems a more reasonable implementation

This doesn't handle the case `desktopStyle.isEmpty()`, but I'm not sure that can ever occur under normal circumstances?
Comment 2 Jean-Baptiste Mardelle 2016-01-12 23:22:17 UTC
I understand and agree that an application is not supposed to override the desktop settings.
However, this change was made because many users requested it. 
Most of Kdenlive's users like to use it with a dark color theme, and many of them use Ubuntu.
On Ubuntu, the default theme for KDE apps is GTK+ which does not work with dark themes. Many widgets keep a light background color, making Kdenlive look really bad if not completely useless (changing color scheme with GTK+ doesn't update text color so you end up with white text on light gray background). As a result, may first time users of Kdenlive on Ubuntu just gave up and declared it worthless. When running a Gnome session, there is no way to change the default widget style.

So currently to me it looks like the choice is to either respect the Desktop style and provide a terrible user experience or override the Desktop style...

Apart from that I agree that the "Default" entry should follow the Desktop setting.
But currently I would still like to use a "Factory" setting (Breeze) by default for Ubuntu users... 
any idea or feedback is welcome as I am aware that this is not a good solution, maybe this should be discussed on a KDE mailing list...
Comment 3 RJVB 2016-01-13 10:21:50 UTC
Agreed to take this to a ML ... if it's one I'm already on ;) Maybe the general ML (kde@mail.kde.org) ?

It sounds like what you would need is a check on startup if the current desktop theme is the GTk+ theme. I presume it shows up under a distinctive name, in `desktopStyle`. If so you would probably be set with something along the lines:

if (!userStyle.isEmpty())
    if (userStyle != desktopStyle)
        // apply the user's kdenlive-specific style selection
else if (desktopStyle == "GTK+" /* or any other style to avoid by default */ )
    // apply an appropriate KDE style, falling back to Fusion if Breeze and/or Oyxgen don't exist

I don't have the sourcecode at hand so I've made some educated guesses about variable names and how to determine if the user indeed selected a style.
 
About Fusion: I can't check right now, but I think I didn't see it show up in the list I got. Which is surprising since it should always be present and is actually not bad at all for a style.
Comment 4 Jean-Baptiste Mardelle 2016-01-13 18:13:57 UTC
Hi,

Yes, thanks for your feedback. I just checked Fusion and it effectively works very well.
I made a quick page comparing Kdenlive's appearance across different styles / color themes here:

http://j-b-m.ch/themes/

Looks like your idea to simply avoid the "GTK" theme would be a better solution.
So I guess I will implement it with something quite close to what you suggest:
if GTK theme is detected, switch to Fusion (or Breeze if Fusion is not available).

I will also edit the "Default" entry to make it obey the Desktop settings.

I will post a short resume of this thread on the kde-devel mailing list for eventual feedback (the kde ML does not really make sense, it is targeting users). If something important shows up, I will post it here.
Comment 5 RJVB 2016-01-13 20:06:22 UTC
Below is an extract of the local patch I'm using for respecting the default style. I didn't implement the use of Fusion myself (that patch is from yesterday when I hadn't yet thought of Fusion :))
I wouldn't have thought of using Fusion before Breeze (Fusion should always be present) but that's up to you to decide.

If you give me a heads-up when you're done I'll check with the native Mac theme. It works better with KDE than it did under KDE4 but it looks certain elements (the play control buttons in the monitor view for instance) tend to get messed up.

diff --git src/mainwindow.cpp src/mainwindow.cpp
index 8f20c3a..aa7f966 100644
--- src/mainwindow.cpp
+++ src/mainwindow.cpp
@@ -124,6 +124,15 @@ QMap <QString,QImage> MainWindow::m_lumacache;
     return a.first < b.first;
 }*/
 
+// determine the the default KDE style as defined BY THE USER
+// (as opposed to whatever style KDE considers default)
+static QString defaultStyle(const char *fallback=Q_NULLPTR)
+{
+    KSharedConfigPtr kdeGlobals = KSharedConfig::openConfig(QStringLiteral("kdeglobals"), KConfig::NoGlobals);
+    KConfigGroup cg(kdeGlobals, "KDE");
+    return cg.readEntry("widgetStyle", fallback);
+}
+
 MainWindow::MainWindow(const QString &MltPath, const QUrl &Url, const QString & clipsToLoad, QWidget *parent) :
     KXmlGuiWindow(parent),
     m_timelineArea(NULL),
@@ -3401,7 +3410,11 @@ void MainWindow::slotChangeStyle(QAction *a)
 
 void MainWindow::doChangeStyle()
 {
-    QApplication::setStyle(QStyleFactory::create(KdenliveSettings::widgetstyle()));
+    QString newStyle = KdenliveSettings::widgetstyle();
+    if (newStyle.isEmpty() || newStyle == QStringLiteral("Default")) {
+        newStyle = defaultStyle("Breeze");
+    }
+    QApplication::setStyle(QStyleFactory::create(newStyle));
     // Changing widget style resets color theme, so update color theme again
     ThemeManager::instance()->slotChangePalette();
 }
Comment 6 Jean-Baptiste Mardelle 2016-01-13 20:33:19 UTC
Git commit e1bdc57f12fc193523ec656f46ae5cc1118044f0 by Jean-Baptiste Mardelle.
Committed on 13/01/2016 at 20:32.
Pushed by mardelle into branch 'Applications/15.12'.

Follow Desktop style unless it is GTK+.
"Default" option follows Desktop settings
Based on a patch by René J.V. Bertin

M  +27   -10   src/mainwindow.cpp

http://commits.kde.org/kdenlive/e1bdc57f12fc193523ec656f46ae5cc1118044f0
Comment 7 Unknown 2016-01-13 21:40:55 UTC
JB, the Fusion widget style you showed in your link looks pretty darn good -- quite close to the original Breeze style. If Breeze isn't decided to be the most appropriate default widget style, and Fusion can be set as the default widget style for Kdenlive when using a GTK-based desktop environment, I think that'd be a great option, compared to the GTK+ widget style (which makes the tabs and other widget decor look absolutely awful).

If that can be implemented, I think it'd be one step closer towards being able to set a dark theme by default, therefore maintaining the application brand consistency, while keeping a nice aesthetic look and feel for users across different desktop environments. :)
Comment 8 Wegwerf 2016-07-31 09:18:34 UTC
RJVB, is this bug report still relevant? If not, I would like to ask you to be so kind as to close it. Thank you very much for your cooperation!