search

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

Dynamically load libfm-qt on demand to create the file dialog helper. #39

Closed PCMan closed 5 years ago

PCMan commented 5 years ago

This requires: https://github.com/lxqt/libfm-qt/pull/243

PCMan commented 5 years ago

@agaida @tsimonq2 With this change, building lxqt-qtplugin no longer require libfm-qt. So people who don't like libfm-qt can live safely without it. Qt programs that don't need the file dialogs can avoid loading libfm-qt as well.

However, I'd really suggest that you add libfm-qt to "recommended" for your lxqt-qtplugin *.deb packages to make them installed together by default since with libfm-qt you can have a much more functional file dialog.

PCMan commented 5 years ago

@agaida @tsujan @yan12125 As mentioned earlier, with this PR all future ABI changes in libfm-qt won't require the rebuild of lxqt-qtplugin. :-)

agaida commented 5 years ago

@PCMan - there is a problem with Recommend: - some really clever guys do to prevent all that "bloat" 

apt install $foo --no-install-recommends

and are whining about missed functionality afterwards - as long they stay with pcmanfm-qt - no problem. And i really don't care about Installations without our main components, so a Recommend: libfm-qt? is the way to go.

tsujan commented 5 years ago

@agaida I think recommending libfm-qt is enough. The "worst" thing that could happen is that the user won't see a feature-rich file dialog without libfm-qt.

I'll test the changes tomorrow.

agaida commented 5 years ago

@tsujan - recommend is enough - and as long the users don't build the system from the scratch it would be damn hard to avoid libfm-qt and pcmanfm-qt firsthand.

agaida commented 5 years ago

@PCMan - Re: needed rebuilds - never did it by hand for debian - thats what transitions are for. But you are right, that should be mentioned in the packaging notes with a big fat exclamation mark. It is important for all our friends who build directly from git (or use the AUR or similar things).

luis-pereira commented 5 years ago

Will review and test Monday, only.

-- Cumprimentos Regards

Em 28/07/2018 22:45, "Alf Gaida" notifications@github.com escreveu:

@PCMan https://github.com/PCMan - Re: needed rebuilds - never did it by hand for debian - thats what transitions are for. But you are right, that should be mentioned in the packaging notes with a big fat exclamation mark. It is important for all our friends who build directly from git (or use the AUR or similar things).

— You are receiving this because your review was requested.

Reply to this email directly, view it on GitHub https://github.com/lxqt/lxqt-qtplugin/pull/39#issuecomment-408636671, or mute the thread https://github.com/notifications/unsubscribe-auth/AAD1naux8o2L8nUlo0DXlH42yDMKsj0iks5uLNtkgaJpZM4VlEMC .

tsujan commented 5 years ago

@agaida I know this is off topic and so, I won't continue it here but, IMO, an LXQt meta package that provides a good LXQt experience could solve your problem with those "clever guys".

agaida commented 5 years ago

@tsujan: not exactly - as long the clever guys use at least lxqt-core without recommends - all things are fine. If this package is still to much "bloat" and they use the packages without the meta-packages things might go south. But thats not a common support case - in case they use the packages alone they should know what they are doing - at least i hope so. So no problem at all.

tsujan commented 5 years ago

Applied both patches. Unfortunately, all Qt apps show the the default Qt file dialog; there's no trace of LXQt file dialog anymore.

tsujan commented 5 years ago

@agaida Do you see the same thing?

agaida commented 5 years ago

- i guess that is our file dialog

tsujan commented 5 years ago

@agaida If you mean you get it with all Qt apps, I should have made a mistake somewhere...

agaida commented 5 years ago

works with nomacs, lximage-qt, vokoscreen, virtualbox but not not with featherpad

agaida commented 5 years ago

ah - featherpad has't had the hook in systemdialog

tsujan commented 5 years ago

@agaida It's lxqt-archiver in your screenshot; it uses lxqt file dialog explicitly.

I hadn't made a mistake; something's definitely wrong.

tsujan commented 5 years ago

Falkon (formerly QupZilla), FeatherPad and FeatherNotes use the Qt file dialog, while they use the native file dialog under KDE and Gnome -- they also use the LXQt file dialog after I reversed the changes a few minutes ago.

PCMan commented 5 years ago

@tsujan do you install libraries to custom dirs? It's possible that libfmqt is not found by qlibrary.

You can use LD_DEBUG=all env var to see what's happening.

What I did is just dynamically load the lib at runtime.

tsujan commented 5 years ago

do you install libraries to custom dirs?

No I don't. @agaida sees the same issue with some Qt apps.

agaida commented 5 years ago

@tsujan - no, but fp was set to use the native dialog

agaida commented 5 years ago

So - for me there is only one remaining problem that has nothing to do with the current PRs. Most of the file open dialogs are full screen and not sizable - guess i had filed a bug about before.

agaida commented 5 years ago

and i still have a problem with linking fm_qt in src/CMakeLists.txt - one will not notice that with local builds, but will do so in a build chroot.

tsujan commented 5 years ago

no, but fp was set to use the native dialog

You mean it was set to NOT use the native dialog -- which isn't the default.

I tried with every Qt app I had; the theory was proved in the practice: the library was loaded and the symbol was resolved.

tsujan commented 5 years ago

Most of the file open dialogs are full screen and not sizable

That should be related to the WM you use: it makes large windows full-screen. Close all file dialogs, remove ~/.config/lxqt/filedialog.conf (or just change WindowSize inside it) and retry.

If my guess is correct, we might need to prevent LXQt file dialog from becoming full-screen OR add F11 to it for making it (un-)full-screen.

agaida commented 5 years ago

@tsujan - you was right about the dialog, deleted the size line, now it work as expected. right now is WindowSize=@Size(1041 738) - and the Window is sizable again - and i found a very special kind of bug - will make some screenshots, hard to explain.

tsujan commented 5 years ago

@agaida Let's discuss it at https://github.com/lxqt/libfm-qt/issues/244

PCMan commented 5 years ago

@agaida Thanks for the reminder. I forgot to delete the explicit "fm-qt linkage" in src/CMakeLists.txt. Just fixed it. It should build without problems now.

yan12125 commented 5 years ago

Sorry for the late participation! I have no access to my Linux box during the weekend. After updating both libfm-qt and lxqt-qtplugin and tested Quassel and KeepassXC, the file choosing dialog looks as awesome as always.

Now here's the question regarding packaging. I'm maintaining the AUR package lxqt-qtplugin-git for Arch Linux users. Currently libfm-qt-git is a hard dependency. Do you think it's a good idea to move that to a optional dependency? Note that by default, pacman (the package manager of Arch Linux) does not install optional dependencies.

tsujan commented 5 years ago

@yan12125 Yes, libfm-qt should be an optional dependency now. The problem of installing optional dependencies in Arch is like that of installing Recommends in Debian ;) Not our problem though.

yan12125 commented 5 years ago

Got it and done! https://aur.archlinux.org/cgit/aur.git/commit/?h=lxqt-qtplugin-git&id=fce9af42d44d76a9a6aa00429c309fbc864d7d59

2018-07-30 19:43 GMT+08:00 tsujan notifications@github.com:

@yan12125 https://github.com/yan12125 Yes, libfm-qt should be an optional dependency now. The problem of installing optional dependencies in Arch is like that of installing Recommends in Debian ;) Not our problem though.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lxqt/lxqt-qtplugin/pull/39#issuecomment-408835545, or mute the thread https://github.com/notifications/unsubscribe-auth/AB2RGakvvVWn-jmOgf3KDPcpu1avt8WIks5uLvF4gaJpZM4VlEMC .

PCMan commented 5 years ago

@yan12125 it's better to make it optional, but mention in the document that it's highly recommended to install libfm-qt and if they don't they will not have the full-featured file dialog. :-)

agaida commented 5 years ago

Nobody read documentations - but as long one install pcmanfm-qt as filemanager libfm-qt is installed anyways - same for lximage-qt. That leave only one corner case: One don't install pcmanfm-qt nor lximage-qt and can't read - i would call this combination simply bad luck. And i guess that a Qt-filedialog is the slightest problem in that special case. :dark_sunglasses:

tsujan commented 5 years ago

i would call this combination simply bad luck

Or, if we put it less politely but more realistically, the user's ignorance ;)

agaida commented 5 years ago

hmmm ... erm ... äh... - yes

tsimonq2 commented 5 years ago

Belated ack, thanks everyone.

yan12125 commented 5 years ago

I've update the description for optional dependencies. Now Arch users will see the following texts when installing lxqt-qtplugin-git from AUR:

Optional dependencies for lxqt-qtplugin-git
    libfm-qt-git: for enhanced file dialog (highly recommended)

Anyway, I can do nothing if they ignore these lines :)

agaida commented 5 years ago

those who can read, write and understand have a real advantage

But: Dsylexics have more fnu.