Make fm-qt a build dependency

[agaida]

Alternative approch to @luis-pereira, introduce a option - if ON (default) be strict about the usage of fm-qt, no fallbacks and no magic allowed.

[agaida]

upps, forget to set the minimum version

[tsujan]

I agree with @luis-pereira.

[agaida]

@tsujan - think about again - we can just require fm-qt in cmake. This is the only way to manifest our will. In that case i suggest a popup with "FM-Qt not found. Check your packaging!" at run-time and not silently falling back to the Qt diaolg.

[luis-pereira]

@tsujan - think about again - we can just require fm-qt in cmake. This is the only way to manifest our will. In that case i suggest a popup with "FM-Qt not found. Check your packaging!" at run-time and not silently falling back to the Qt diaolg.

It libfm-qt is required I have no problem with that. The fallback made sense when libfm-qt was not required.

[tsujan]

@agaida lxqt-qtplugin is for our platform integration, of which LXQt file dialog is a part. If a distributor makes a big mistake and doesn't upgrade lxqt-qtplugin after a new release but upgrades libfm-qt, it'll be his responsibility, not ours — to say nothing about an outdated lxqt-qtplugin that might not work at all.

[agaida]

@tsujan - agree - the problem remains two-fold. If i understand you right:

• We require fm-qt at build time. No option, no fallback - just FTBFS, if not there (Step 1)
• Notify people if required things are not found at run-time (Step 2)

[tsujan]

@agaida If our imaginary distributor makes another mistake and compiles lxqt-qtplugin without libfm-qt, no disaster will happen; his package will just be not-good-enough for LXQt.

However, I can repeat @luis-pereira's reply because I'd say the same thing: If libfm-qt is required, your patch will be good to me.

[agaida]

            QLibrary libfmQtLibrary{QLatin1String("libfm-qt"), QLatin1String(FMQT_SO_VERSION)};
            libfmQtLibrary.load();
            if(!libfmQtLibrary.isLoaded()) {
                return nullptr;

As long these 4 lines exist in our platform plugin fm-qt is optional, like it or not.

And thats the reason why i made fm-qt optional with default ON in cmake

[tsujan]

Summing it up:

We have 2 good options:

(1) Not requiring libfm-qt, in which case, @luis-pereira's patch is the best solution, IMO, but distributors might make mistakes (although it' unlikely). (2) Requiring libfm-qt, in which case, no distributor could make a mistake and we'll never receive reports that say, "why don't I see LXQt file dialog?"

[tsujan]

As long these 4 lines exist in our platform plugin fm-qt is optional

So, (2) (in my previous comment) has this "drawback" that it makes cmake not so compatible with the code.

Decision is yours and @luis-pereira's.

[agaida]

Don't get me wrong - i don't want fm-qt optional - but as long lxqt-qtplugin use no symbol from fm-qt and is loaded dynamical it will remain optional - we can only say: Hey, we want it to be used. We can't force it with the current code. Therefor the option. And the release notes must contain a reminder to the packagers to add fm-qt to the dependencies.

[tsujan]

The current patch changes what I called as "distributors' unlikely mistakes" to something legitimate ;)

All this being said, I just told my opinion and will agree with any decision that might be taken.

[luis-pereira]

IMO the only way lxqt-qtplugin "should" be used without the libfm-qt dialog is by mistake. We should not legitimate it. The dialog is part of the platform plugin.

If someone makes an unlikely error or make it on purpose... There's nothing we can do about it.

find_package(fm-qt REQUIRED)

is a very strong hint to fm-qt is required.

Regarding the load at runtime... there's nothing we can do about it. We don't want to link to it.

[luis-pereira]

Of course the release notes should contain a reminder to packagers

[agaida]

Ok, based on the discussion i will change a bit of the code

[agaida]

So the REQUIRED should give a strong hint about libfm-qt :)

[agaida]

@tsujan @luis-pereira @PCMan - what do you think?

[tsujan]

I have nothing against it. What you said made me think that requiring libfm-qt might create a problem in Debian but you know those things better than anyone.

[tsujan]

Or said more positively, no user will encounter an incomplete platform integration :)

[agaida]

diff --git a/debian/control b/debian/control
index 5ecaed3..b0e7234 100644
--- a/debian/control
+++ b/debian/control
@@ -7,6 +7,7 @@ Section: x11
 Priority: optional
 Build-Depends: debhelper-compat (= 12),
                libdbusmenu-qt5-dev,
+               libfm-qt-dev (>= 0.14.2),
                libkf5windowsystem-dev,
                libqt5svg5-dev,
                libqt5x11extras5-dev,
@@ -23,9 +24,9 @@ Package: lxqt-qtplugin
 Architecture: any
 Multi-Arch: same
 Depends: ${misc:Depends},
-         ${shlibs:Depends}
-Recommends: libfm-qt6,
-            lxqt-session,
+         ${shlibs:Depends},
+         libfm-qt6
+Recommends: lxqt-session,
             lxqt-config
 Suggests: lxqt | lxqt-core
 Description: LXQt system integration plugin for Qt

That's all, nothing to see here.

[tsujan]

IMO, it's good — (2) in https://github.com/lxqt/lxqt-qtplugin/pull/49#issuecomment-489173581

[agaida]

Current state is that i only recommend libfm-qt6 - so if very clever users install without recommends - not my fault, it will fall back to the Qt dialog. We had discussed it several times. But i will close related bugs without discussion.

Edit: This has to be very clever users who don't have pcmanfm-qt installed - so it will work for 99,99% of all installations in Buster.

[tsujan]

Oh, don't forget to test it with a Debian ISO without dev packages.

EDIT: And don't forget to change the title ("Optional fmqt").

[agaida]

already in Buster: https://salsa.debian.org/lxqt-team/lxqt-qtplugin/blob/debian/sid/debian/patches/load-versioned-libfm-qt.patch

-            QLibrary libfmQtLibrary{QLatin1String("libfm-qt")};
+            QLibrary libfmQtLibrary{QLatin1String("libfm-qt.so.6")};

[tsujan]

I meant the current patch; for the sake of super certainty ;)

[agaida]

sure - and if anything fails we have approx. two years to fix it for Bullseye :D

[agaida]

Ok, just tested in debian buster - apply, build and work fine

[agaida]

GTM - @tsujan, @luis-pereira - merge it?

[tsujan]

Yes, IMO.

[agaida]

bah - thought i had squash chosen - but maybe better if we can see later why we implemented it that way.

[luis-pereira]

That's why we should use for git merge --no-ff