Native look of file choosing dialog on GTK Linux systems

[BOOtak]

[BOOtak]

Screenshots attached

[auchri]

Fixes #982 #1140 #1404 #929

[vermeeren]

I'm not sure how this affects the possibility of -no-gtkstyle being removed to fix the long-standing -style=0 problem that has been an issue for a while.

It also appears that the GTK version used for the file picker is now 2 instead of 3, which is afaik not the preferred choice for a DE that natively uses 3.

This still doesn't give a native filepicker for e.g. KDE and probably other DEs.

Consider the following example.

#include <QtWidgets/QApplication>
#include <QtWidgets/QFileDialog>

int main(int argc, char **argv)
{
    QApplication *qa = new QApplication(argc, argv);

    QFileDialog::getOpenFileName
    (
        nullptr,
        "Open File",
        "/home",
        "Images (*.png *.xpm *.jpg)"
    );

    delete qa;
    qa = nullptr;

    return 0;
}

compile it with and run ./a.out

g++ -I/usr/include/qt -fPIC -std=gnu++14 -lQt5Core -lQt5Widgets main.cpp

As you can see, the native system filepicker is used automatically. In my case KDE 5's file picker. I suspect this behaviour is consistent across OSes and DEs, since Qt prefers native when possible.
http://doc.qt.io/qt-5/qfiledialog.html#getOpenFileName
http://stackoverflow.com/a/6405275

Perhaps it is a better idea to let Qt itself choose the system native dialog. It is only when a lot of QFileDialog options are specified that Qt is FORCED to use the built-in dialog since it only then can be absolutely sure that the requested options are available.

If you ask me this is the solution. Less option forcing and hacking around a problem by inserting 3 other problems at the same time.

View #987 (comment) for some background and the possible fix for -style=0.

[vermeeren]

Asked a friend using Gnome 3 to also try the above code.

[ManuelSchneid3r]

Is it just me, or is this PR basically just removing everything o GTK3?

Note: Qt 5.7 does heavily mess up qt - gtk interaction, by moving GTK+ style to the qtstyles plugin.

[BOOtak]

@ManuelSchneid3r it's more you then PR as Qt 5.7 is unsupported by TDesktop client. Compile project with 5.6 and everything will be just fine.

[BOOtak]

@melvinvermeeren TL;DR: please read project's source code carefully before writing comments next time. I have no idea why I should spend my time to explain obvious things.

It also appears that the GTK version used for the file picker is now 2 instead of 3

No. I've changed GTK version for absolutely different reason (read my commit message carefully) and it has no connection at all with file picker's theme.

This still doesn't give a native filepicker for e.g. KDE and probably other DEs.

Again, qgtk2 is a theme engine and will be applied only to GTK file picker if it will be used. As written in Qt docs, By default, a platform-native file dialog will be used if the platform has one which is not GTK file picker on KDE. So, my PR has no connection to and no influence on native KDE file picker. I have no idea why KDE doesn't use it's own file picker but I strongly confident that it's completely separate issue and I can't wait for your PR to fix this.

Perhaps it is a better idea to let Qt itself choose the system native dialog. It is only when a lot of QFileDialog options are specified that Qt is FORCED to use the built-in dialog since it only then can be absolutely sure that the requested options are available.

I see you haven't read project source code at all as project's QFileDialog utilizes only common set of options which every native implementation has for sure.

Consider the following example.
g++ -I/usr/include/qt -fPIC -std=gnu++14 -lQt5Core -lQt5Widgets main.cpp

Your example will be compiled with system Qt which already built with all possible platforms ant theme engines. Tdesktop uses it's own static version of Qt patched and built from source code. Let me enhance your example and show what would happen if we compile it with static Qt:

QT += core gui network widgets

CONFIG += plugin static c++14

QT_TDESKTOP_VERSION = 5.6.0
QT_TDESKTOP_PATH = $$(QT_TDESKTOP_PATH)

INCLUDEPATH += $${QT_TDESKTOP_PATH}/include/QtGui/$${QT_TDESKTOP_VERSION}/QtGui \
               $${QT_TDESKTOP_PATH}/include/QtCore/$${QT_TDESKTOP_VERSION}/QtCore \
               $${QT_TDESKTOP_PATH}/include

SOURCES += \
    ./test.cpp

INCLUDEPATH += \
               /usr/local/include

QTPLUGIN.platformthemes += qgtk2

LIBS += $${QT_TDESKTOP_PATH}/plugins/platforminputcontexts/libcomposeplatforminputcontextplugin.a \
        $${QT_TDESKTOP_PATH}/plugins/platforminputcontexts/libibusplatforminputcontextplugin.a \
        $${QT_TDESKTOP_PATH}/plugins/platforminputcontexts/libfcitxplatforminputcontextplugin.a

This is test.pro file copy-pasted from Telegram.pro with slight modification (some parts are thrown away).

Let's build project:
qmake test.pro
make

And launch it:
./test

Here is the result:

WTF, right?
And now we add one line to our test.pro:

QTPLUGIN.platformthemes += qgtk2

And here is the result:

So, solution provided in this PR only applies correct theme to GTK file picker, no less, no more. It doesn't influence KDE or other platforms which have their own picker.

[vermeeren]

@BOOtak Thanks for explaining more about the state of things with tdesktop itself, I didn't know all of this yet.

Even so, instead of changing even more inside tdesktop itself, wouldn't it be a better idea to change the configuration of the static Qt build to include support for native filepickers in the same way the system Qt does? Even if this increases the binary by a few M it shouldn't pose a problem.

I just can't help but feel that the problems are being solved in a nonstandard ways and will most likely cause further issues later on, there have been too many cases of this already with tdesktop imo. Kind of off-topic, but I'd like to see a truly native tdesktop one day.

[john-preston]

@BOOtak I don't like the idea of removing the "appindicator3" loading (which requires "gtk-3", if I understand correctly the difference between "appindicator" and "appindicator3" libraries).

For example, Ubuntu 15.04 has libappindicator3.so but doesn't have libappindicator.so, so removing the gtk-3 version of libappindicator will turn off the tray icon support on all the systems, that rely on libappindicator3.so, which I think is not acceptable.

[BOOtak]

@john-preston

requires "gtk-3", if I understand correctly the difference between "appindicator" and "appindicator3" libraries

Exactly!

removing the gtk-3 version of libappindicator will turn off the tray icon support on all the systems,

That is an issue indeed. Is it clear why I've removed gtk-3 libs? (Hint: to avoid mixing gtk-2 and gtk-3 symbols which would cause app's crash). So, static linking with libappindicator should do the trick/ i'll rebase my changes on top of latest master branch and try to link appindicator statically.

[john-preston]

libappindicator can't be statically linked as I know (at least I could not do that at the time I tried), so I started to load it dynamically and load the needed version of gtk library.

I'm afraid the only way to make things working with both appindicator, appindicator3 and gtk file picker is to write specific code for gtk file picker usage instead of Qt file pickers, which will work both with dynamically loaded gtk2 lib and gtk3 lib :(

PS I guess that is the reason I had to add "-style=0" synthetic argument to QApplication, because if not added Qt tried to load gtk library dynamically -- sometimes loading gtk2 while I loaded gtk3 for appindicator3 -- and everything crashed :/

[john-preston]

@BOOtak I don't see this comment on github.. I call methods from libgtk together with libappindicator methods, I guess they should be from the same version of libgtk.

I'll try to implement (copy paste :) my own gtk file dialog that'll work with both gtk2 and gtk3, I hope this will work.

[john-preston]

Another problem is that adding "QTPLUGIN.platformthemes += qgtk2" statically links a huge amount of libraries, which were not in dependencies for the currently deployed build.

[BOOtak]

@john-preston those new libs should be mentioned in documentation and travis build agent as soon as we handle all other issues.
PS still trying to link appindicator statically (bulid it from source as static lib)

[john-preston]

@BOOtak The problem with them is not the inconsistency of docs or travis failing. The problem is that the binary may stop launching on some systems where it does work now. A new version can't be deployed that will just stop working on some systems.

[john-preston]

I've pushed to master branch a version with self-made GtkFileDialog (based on Qt theme plugin code), it works for me in Ubuntu 12.04 for both gtk-2 and gtk-3.

[BOOtak]

That is definitely better solution than mine! I'll test that. Thank you anyway for paying attention to this problem as previous look of file choosing was horrible :)

[john-preston]

In the latest 0.9.57 version gtk file chooser should be used if it is available. Please check.

[kvaster]

File chooser on gentoo with KDE is broken now. GTK file chooser is also available, cause I have some GTK apps installed, but native file chooser is really much better.

[john-preston]

@kvaster was it native file chooser before? All systems I've tested with old tdesktop version were using Qt inner file chooser implementation, that was not like "kdialog" in KDE, for example. Can you test

https://telegram.org/dl/desktop/linux (without updating to alpha -- disable autoupdate in Settings)

version and say if it is really better (native?), than the new 0.9.57 alpha? If it does -- maybe you can tell me, how I can understand in the code (read some env variable?) do I need to try to use GTK file chooser or not.

[kvaster]

Seems you're right - it was opening Qt inner file chooser.

http://imgur.com/a/j7EiJ

I will try to figure out today if there is reliable method to detect KDE and to open Qt file chooser instead of gtk. On kde Qt chooser looks much better in my opinion.

[AndydeCleyre]

@kvaster

if there is reliable method to detect KDE

$ echo $DESKTOP_SESSION

/usr/share/xsessions/plasma

$ echo $KDE_FULL_SESSION

true

From KDE docs on KDE_FULL_SESSION:

Set to true by KDE startup, it is used by some programs, such as Konqueror, to know if they should consider remaining in memory for future re-use when being closed. If not set, those programs will exit after being closed (e.g. kdesu does that, it's also useful for debugging).

If you plan on using this variable to detect a running KDE session, check if the value is not empty instead of seeing if it equals true. The value might be changed in the future to include KDE version information.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.