diff mbox series

[bug#39102,2/2,staging] gnu: qtbase: Open links properly without xdg-utils in profile

Message ID 20200112154709.fhxirioephom3zvc@zdrowyportier.kadziolka.net
State Accepted
Headers show
Series [bug#39102,1/2] gnu: xdg-utils: Don't use propagated inputs. | expand

Checks

Context Check Description
cbaines/applying patch success Successfully applied

Commit Message

Maja Kądziołka Jan. 12, 2020, 3:47 p.m. UTC 
* gnu/packages/patches/qtbase-use-xdg-open-in-store.patch: New file.
* gnu/packages/qt.scm (qtbase)[source][patches]: Apply the patch.
  [inputs]: Add a dependency on xdg-utils to get its store path.
  [arguments]: Add a new phase to patch the path into the source code.
---
 .../qtbase-use-xdg-open-in-store.patch        | 103 ++++++++++++++++++
 gnu/packages/qt.scm                           |  10 +-
 2 files changed, 112 insertions(+), 1 deletion(-)
 create mode 100644 gnu/packages/patches/qtbase-use-xdg-open-in-store.patch

Comments

Marius Bakke Jan. 12, 2020, 7:44 p.m. UTC | #1 
Jakub Kądziołka <kuba@kadziolka.net> writes:

> * gnu/packages/patches/qtbase-use-xdg-open-in-store.patch: New file.
> * gnu/packages/qt.scm (qtbase)[source][patches]: Apply the patch.
>   [inputs]: Add a dependency on xdg-utils to get its store path.
>   [arguments]: Add a new phase to patch the path into the source code.

This patch does a lot.  :-)

With this patch, BROWSER and DEFAULT_BROWSER would no longer be
consulted, right?

Does checkExecutable work with absolute file names?  I.e. could we get
away by simply patching "xdg-open" with its store file name?  Probably
should change the default browsers while at it, though.  :-)

Wrt the easy substitution, I think we should try and avoid introducing
changes to source code that depend on magic from #:phases.  That way
people will still be (mostly) able to build Qt manually using the Guix
source.  In this case, maybe defaulting to just "xdg-open" is enough?

In short, I'm looking for an easier way to achieve the same goal,
without the rather intrusive patch.

Copying Efraim as our resident Qt expert.
Efraim Flashner Jan. 13, 2020, 7:53 a.m. UTC | #2 
On Sun, Jan 12, 2020 at 08:44:43PM +0100, Marius Bakke wrote:
> Jakub Kądziołka <kuba@kadziolka.net> writes:
> 
> > * gnu/packages/patches/qtbase-use-xdg-open-in-store.patch: New file.
> > * gnu/packages/qt.scm (qtbase)[source][patches]: Apply the patch.
> >   [inputs]: Add a dependency on xdg-utils to get its store path.
> >   [arguments]: Add a new phase to patch the path into the source code.
> 
> This patch does a lot.  :-)
> 
> With this patch, BROWSER and DEFAULT_BROWSER would no longer be
> consulted, right?
> 
> Does checkExecutable work with absolute file names?  I.e. could we get
> away by simply patching "xdg-open" with its store file name?  Probably
> should change the default browsers while at it, though.  :-)
> 
> Wrt the easy substitution, I think we should try and avoid introducing
> changes to source code that depend on magic from #:phases.  That way
> people will still be (mostly) able to build Qt manually using the Guix
> source.  In this case, maybe defaulting to just "xdg-open" is enough?
> 
> In short, I'm looking for an easier way to achieve the same goal,
> without the rather intrusive patch.
> 
> Copying Efraim as our resident Qt expert.

I disagree about being a qt expert, I just like fixing packages :)

Looking at the patch, I'm not in love with how there's a default list of
browsers to look for. Looking at the code, it seems that if there's
xdg-open available then open browser from the pre-defined list.

What is the code around m_documentLauncher? Does that really need to be
removed?

I think our best bet would be to substitute xdg-open with the actual
xdg-open binary around line 130 and to change the list of *browsers[] to
ones we actually have in Guix. If we switched the list to {"icecat",
"next", "chromium", "netsurf"} and made the substitution on xdg-open
we'd be in a much better place than we are now.

As it currently stands I know I don't have BROWSER or DEFAULT_BROWSER
defined and we don't have any Debian-like package named
'default-browser' or similar that we could throw in.
diff mbox series

Patch

diff --git a/gnu/packages/patches/qtbase-use-xdg-open-in-store.patch b/gnu/packages/patches/qtbase-use-xdg-open-in-store.patch
new file mode 100644
index 0000000000..505fff13c2
--- /dev/null
+++ b/gnu/packages/patches/qtbase-use-xdg-open-in-store.patch
@@ -0,0 +1,103 @@ 
+To make Qt respect the user's settings for the preferred browser, use xdg-open
+even if it is not installed in the profile. The patch makes Qt always use
+xdg-open, and makes the path used easy to substitute when building.
+========================================================================
+--- qtbase-everywhere-src-5.12.6.orig/src/platformsupport/services/genericunix/qgenericunixservices.cpp	2020-01-01 20:47:16.775671802 +0100
++++ qtbase-everywhere-src-5.12.6/src/platformsupport/services/genericunix/qgenericunixservices.cpp	2020-01-01 20:56:22.167662035 +0100
+@@ -117,47 +117,6 @@
+     return QByteArrayLiteral("UNKNOWN");
+ }
+ 
+-static inline bool checkExecutable(const QString &candidate, QString *result)
+-{
+-    *result = QStandardPaths::findExecutable(candidate);
+-    return !result->isEmpty();
+-}
+-
+-static inline bool detectWebBrowser(const QByteArray &desktop,
+-                                    bool checkBrowserVariable,
+-                                    QString *browser)
+-{
+-    const char *browsers[] = {"google-chrome", "firefox", "mozilla", "opera"};
+-
+-    browser->clear();
+-    if (checkExecutable(QStringLiteral("xdg-open"), browser))
+-        return true;
+-
+-    if (checkBrowserVariable) {
+-        QByteArray browserVariable = qgetenv("DEFAULT_BROWSER");
+-        if (browserVariable.isEmpty())
+-            browserVariable = qgetenv("BROWSER");
+-        if (!browserVariable.isEmpty() && checkExecutable(QString::fromLocal8Bit(browserVariable), browser))
+-            return true;
+-    }
+-
+-    if (desktop == QByteArray("KDE")) {
+-        // Konqueror launcher
+-        if (checkExecutable(QStringLiteral("kfmclient"), browser)) {
+-            browser->append(QLatin1String(" exec"));
+-            return true;
+-        }
+-    } else if (desktop == QByteArray("GNOME")) {
+-        if (checkExecutable(QStringLiteral("gnome-open"), browser))
+-            return true;
+-    }
+-
+-    for (size_t i = 0; i < sizeof(browsers)/sizeof(char *); ++i)
+-        if (checkExecutable(QLatin1String(browsers[i]), browser))
+-            return true;
+-    return false;
+-}
+-
+ static inline bool launch(const QString &launcher, const QUrl &url)
+ {
+     const QString command = launcher + QLatin1Char(' ') + QLatin1String(url.toEncoded());
+@@ -297,6 +256,8 @@
+     return result;
+ }
+ 
++static QString xdg_open = QStringLiteral("@@GUIX:XDG-OPEN@@");
++
+ bool QGenericUnixServices::openUrl(const QUrl &url)
+ {
+     if (url.scheme() == QLatin1String("mailto")) {
+@@ -320,11 +281,7 @@
+     }
+ #endif
+ 
+-    if (m_webBrowser.isEmpty() && !detectWebBrowser(desktopEnvironment(), true, &m_webBrowser)) {
+-        qWarning("Unable to detect a web browser to launch '%s'", qPrintable(url.toString()));
+-        return false;
+-    }
+-    return launch(m_webBrowser, url);
++    return launch(xdg_open, url);
+ }
+ 
+ bool QGenericUnixServices::openDocument(const QUrl &url)
+@@ -337,11 +294,7 @@
+     }
+ #endif
+ 
+-    if (m_documentLauncher.isEmpty() && !detectWebBrowser(desktopEnvironment(), false, &m_documentLauncher)) {
+-        qWarning("Unable to detect a launcher for '%s'", qPrintable(url.toString()));
+-        return false;
+-    }
+-    return launch(m_documentLauncher, url);
++    return launch(xdg_open, url);
+ }
+ 
+ #else
+diff -ur qtbase-everywhere-src-5.12.6.orig/src/platformsupport/services/genericunix/qgenericunixservices_p.h qtbase-everywhere-src-5.12.6/src/platformsupport/services/genericunix/qgenericunixservices_p.h
+--- qtbase-everywhere-src-5.12.6.orig/src/platformsupport/services/genericunix/qgenericunixservices_p.h	2020-01-01 20:47:16.775671802 +0100
++++ qtbase-everywhere-src-5.12.6/src/platformsupport/services/genericunix/qgenericunixservices_p.h	2020-01-01 20:54:12.135664364 +0100
+@@ -65,10 +65,6 @@
+ 
+     bool openUrl(const QUrl &url) override;
+     bool openDocument(const QUrl &url) override;
+-
+-private:
+-    QString m_webBrowser;
+-    QString m_documentLauncher;
+ };
+ 
+ QT_END_NAMESPACE
diff --git a/gnu/packages/qt.scm b/gnu/packages/qt.scm
index 514577678e..0b1d372d8b 100644
--- a/gnu/packages/qt.scm
+++ b/gnu/packages/qt.scm
@@ -14,6 +14,7 @@ 
 ;;; Copyright © 2019, 2020 Marius Bakke <mbakke@fastmail.com>
 ;;; Copyright © 2018 John Soo <jsoo1@asu.edu>
 ;;; Copyright © 2020 Mike Rosset <mike.rosset@gmail.com>
+;;; Copyright © 2020 Jakub Kądziołka <kuba@kadziolka.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -349,7 +350,8 @@  developers using C++ or QML, a CSS & JavaScript like language.")
               (base32
                "09wz7zs1x5mpgs2y4xnl2zv3naqls4sz6v2arwl1fz2dsx4jddba"))
              ;; Use TZDIR to avoid depending on package "tzdata".
-             (patches (search-patches "qtbase-use-TZDIR.patch"))
+             (patches (search-patches "qtbase-use-TZDIR.patch"
+                                      "qtbase-use-xdg-open-in-store.patch"))
              (modules '((guix build utils)))
              (snippet
                ;; corelib uses bundled harfbuzz, md4, md5, sha3
@@ -407,6 +409,7 @@  developers using C++ or QML, a CSS & JavaScript like language.")
        ("xcb-util-keysyms" ,xcb-util-keysyms)
        ("xcb-util-renderutil" ,xcb-util-renderutil)
        ("xcb-util-wm" ,xcb-util-wm)
+       ("xdg-utils" ,xdg-utils)
        ("zlib" ,zlib)))
     (native-inputs
      `(("bison" ,bison)
@@ -428,6 +431,11 @@  developers using C++ or QML, a CSS & JavaScript like language.")
                             "qmake/library/qmakebuiltins.cpp")
                           (("/bin/sh") (which "sh")))
              #t))
+         (add-after 'configure 'patch-xdg-open
+           (lambda _
+             (substitute* '("src/platformsupport/services/genericunix/qgenericunixservices.cpp")
+                          (("@@GUIX:XDG-OPEN@@") (which "xdg-open")))
+             #t))
          (replace 'configure
            (lambda* (#:key outputs #:allow-other-keys)
              (let ((out (assoc-ref outputs "out")))