Bug 402077 - SaveFile does not set the current_name
Summary: SaveFile does not set the current_name
Status: RESOLVED FIXED
Alias: None
Product: xdg-desktop-portal-kde
Classification: Plasma
Component: general (show other bugs)
Version: unspecified
Platform: Ubuntu Linux
: NOR normal
Target Milestone: ---
Assignee: Jan Grulich
URL:
Keywords:
: 402305 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-12-12 22:39 UTC by Chris Holland
Modified: 2019-02-17 05:16 UTC (History)
10 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.14.5
Sentry Crash Report:


Attachments
Debug output requested by Jan Grulich in comment #7 (80.00 KB, application/x-tar)
2018-12-17 12:56 UTC, 2wxsy58236r3
Details
dbus-monitor log (20.06 KB, text/plain)
2018-12-18 01:01 UTC, Eugene
Details
xdg-desktop-portal-kde log (1.21 KB, text/plain)
2018-12-18 01:09 UTC, Eugene
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Holland 2018-12-12 22:39:36 UTC
As discussed in:
https://www.reddit.com/r/kde/comments/a5cxwk/firefox_v64_can_now_use_the_kde_file_selection/ebneet7/

OS: Manjaro Linux
xdg-desktop-portal: v1.0.3-1
xdg-desktop-portal-kde: v5.14.4-1


Here's where the GTK portal sets "current_name"
https://github.com/flatpak/xdg-desktop-portal-gtk/blob/master/src/filechooser.c#L479-L482

The KDE portal parses "current_name", but it seems that since there wasn't a dedicated "setFilename" function, the fella moved on to implementing the rest.
https://github.com/KDE/xdg-desktop-portal-kde/blob/master/src/filechooser.cpp#L252

    // TODO: Looks Qt doesn't have API for this
    // if (!currentName.isEmpty()) {
    //    fileDialog->selectFile(currentName);
    // }


I'm not 100% sure if this is correct, but I found that QFileDialog can set the "filename" label widget:
http://doc.qt.io/qt-5/qfiledialog.html#DialogLabel-enum
http://doc.qt.io/qt-5/qfiledialog.html#setLabelText

    if (!currentName.isEmpty()) {
       fileDialog->setLabelText(QFileDialog::FileName, currentName);
    }

I'll see about learning how to compile this once I've tested if setLabelText is the correct method.
Comment 1 Chris Holland 2018-12-12 23:38:02 UTC
I was wrong, setLabelText is *suppose* to set the label next to the filename TextField but it seems the the KDE KDialog ignores it.
https://github.com/qt/qtbase/blob/5.11/src/widgets/dialogs/qfiledialog.cpp#L605

Looking at the selectFile logic...
https://github.com/qt/qtbase/blob/5.11/src/widgets/dialogs/qfiledialog.cpp#L1047

It seems that it will set the lineEdit text even if the filename does not exist, and even if it's a relative path.

So I suggest we move the selectFile(currentName) after we navigate to the correct directory.

    if (!currentFolder.isEmpty()) {
        fileDialog->setDirectoryUrl(QUrl(currentFolder));
    }

    if (!currentFile.isEmpty()) {
        fileDialog->selectFile(currentFile);
    }

    if (!currentName.isEmpty()) {
        fileDialog->selectFile(currentName);
    }
Comment 2 Jan Grulich 2018-12-13 06:48:19 UTC
Git commit fa5161c17e6acbe72efe833eb314a5c15183317b by Jan Grulich.
Committed on 13/12/2018 at 06:47.
Pushed by grulich into branch 'Plasma/5.14'.

FileChooser: make use of current_name property in Save dialog

It's not used in Qt portal implementation, but gtk has API for that and sets it in Save dialog
and we should try to call selectFile() with it to get pre-selected file for the dialog

M  +4    -5    src/filechooser.cpp

https://commits.kde.org/xdg-desktop-portal-kde/fa5161c17e6acbe72efe833eb314a5c15183317b
Comment 3 Jan Grulich 2018-12-13 10:06:12 UTC
Git commit ccea985840cf5b05b0359067854abc3da080e737 by Jan Grulich.
Committed on 13/12/2018 at 10:05.
Pushed by grulich into branch 'master'.

FileChooser: make use of current_name property in Save dialog

It's not used in Qt portal implementation, but gtk has API for that and sets it in Save dialog
and we should try to call selectFile() with it to get pre-selected file for the dialog

M  +4    -5    src/filechooser.cpp

https://commits.kde.org/xdg-desktop-portal-kde/ccea985840cf5b05b0359067854abc3da080e737
Comment 4 2wxsy58236r3 2018-12-17 07:25:34 UTC
It does not work for me - after recompiling xdg-desktop-portal-kde from the latest git (commit 23b11bf) and restarting my computer, when I try to save the page (CTRL-S), the file name field is still blank. If I unset GTK_USE_PORTAL=1 (i.e. use GTK file selection dialog), the file name field will be filled in automatically, e.g. `402077 – SaveFile does not set the current_name.html`.
Comment 5 Jan Grulich 2018-12-17 08:20:51 UTC
It does work for me. Are you sure you installed xdg-desktop-portal-kde to correct location?
Comment 6 2wxsy58236r3 2018-12-17 11:38:51 UTC
(In reply to Jan Grulich from comment #5)
> It does work for me. Are you sure you installed xdg-desktop-portal-kde to
> correct location?

I am very sure that xdg-desktop-portal-kde is installed to the correct location. I used the PKGBUILD from https://www.archlinux.org/packages/extra/x86_64/xdg-desktop-portal-kde/ , and made necessary changes to the PKGBUILD to include your patch. Unfortunately the file name field bug is still present.

I am using Firefox 64.0 and xdg-desktop-portal 1.0.3.
Comment 7 Jan Grulich 2018-12-17 11:42:28 UTC
Can you run "dbus-monitor --session" in a terminal while opening the dialog and attach the output here? Also start xdg-desktop-portal-kde in terminal to see the output, using "QT_LOGGING_RULES='xdp*.debug=true' /usr/libexec/xdg-desktop-portal-kde" and attach the output here as well. You will need to kill it first before you can start it again manually, otherwise you just get an error that it failed to register.
Comment 8 2wxsy58236r3 2018-12-17 12:56:18 UTC
Created attachment 116963 [details]
Debug output requested by Jan Grulich in comment #7

Text files containing debug output can be found in the archive file.
Comment 9 Jan Grulich 2018-12-17 13:20:55 UTC
From the logs it looks everything is correct and I can't think of any reason why it doesn't work for you. Maybe try to put some debug there to see if selectFile() is really called in SaveFile() method.
Comment 10 Eugene 2018-12-18 00:55:49 UTC
Hey, the same issue in Kubuntu 18.10
xdg-desktop-portal: 1.0.3-1
xdg-desktop-portal-kde: 5.14.4-0
Comment 11 Eugene 2018-12-18 01:01:00 UTC
Created attachment 116976 [details]
dbus-monitor log
Comment 12 Eugene 2018-12-18 01:09:59 UTC
Created attachment 116977 [details]
xdg-desktop-portal-kde log
Comment 13 Nate Graham 2018-12-18 04:45:25 UTC
(In reply to Eugene from comment #10)
> xdg-desktop-portal-kde: 5.14.4-0

The fix is in 5.14.5, which hasn't been released yet. See the "Version Fixed In" field up towards the top of the page.
Comment 14 Jan Grulich 2018-12-18 14:27:45 UTC
*** Bug 402305 has been marked as a duplicate of this bug. ***
Comment 15 2wxsy58236r3 2019-01-02 06:33:17 UTC
(In reply to Jan Grulich from comment #9)
> From the logs it looks everything is correct and I can't think of any reason
> why it doesn't work for you. Maybe try to put some debug there to see if
> selectFile() is really called in SaveFile() method.

I recompiled xdg-desktop-portal-kde from the latest git (commit 57b2179) and it now works.
Comment 16 AngryPenguin 2019-01-19 02:08:22 UTC
Hi. I tried xdg-desktop-portal-kde 5.14.5 and even the last released 5.14.90 in OpenMandriva Cooker and it does not fix the problem. The file name field is still blank ...

BTW. if needed this is our spec https://github.com/OpenMandrivaAssociation/xdg-desktop-portal-kde

Any idea?
Comment 17 Bernhard Rosenkränzer 2019-01-19 02:09:37 UTC
Not yet working here (xdg-desktop-portal 1.1.1, xdg-desktop-portal-kde 5.14.5, qt 5.12)...

dbus monitor output looks good, relevant part being:

method call time=1547863266.166011 sender=:1.327 -> destination=:1.321 serial=88 path=/org/freedesktop/portal/desktop; interface=org.freedesktop.impl.portal.FileChooser; member=SaveFile
   object path "/org/freedesktop/portal/desktop/request/1_324/gtk582257145"
   string ""
   string "x11:4a00011"
   string "Save As"
   array [
      dict entry(
         string "modal"
         variant             boolean true
      )
      dict entry(
         string "filters"
         variant             array [
               struct {
                  string "Web Page, complete"
                  array [
                     struct {
                        uint32 0
                        string "*.[hH][tT][mM]"
                     }
                     struct {
                        uint32 0
                        string "*.[hH][tT][mM][lL]"
                     }
                  ]
               }
               struct {
                  string "Web Page, HTML only"
                  array [
                     struct {
                        uint32 0
                        string "*.[hH][tT][mM]"
                     }
                     struct {
                        uint32 0
                        string "*.[hH][tT][mM][lL]"
                     }
                  ]
               }
               struct {
                  string "Text Files"
                  array [
                     struct {
                        uint32 0
                        string "*.[tT][xX][tT]"
                     }
                     struct {
                        uint32 0
                        string "*.[tT][eE][xX][tT]"
                     }
                  ]
               }
               struct {
                  string "All Files"
                  array [
                     struct {
                        uint32 0
                        string "*"
                     }
                  ]
               }
            ]
      )
      dict entry(
         string "current_name"
         variant             string "OpenMandriva.html"
      )
      dict entry(
         string "current_folder"
         variant             array of bytes "/home/bero/Downloads" + \0
      )
   ]


xdg-desktop-portal-kde log looks good too:

xdp-kde-file-chooser: SaveFile called with parameters:
xdp-kde-file-chooser:     handle:  "/org/freedesktop/portal/desktop/request/1_324/gtk582257145"
xdp-kde-file-chooser:     parent_window:  "x11:4a00011"
xdp-kde-file-chooser:     title:  "Save As"
xdp-kde-file-chooser:     options:  QMap(("current_folder", QVariant(QByteArray, "/home/bero/Downloads\x00"))("current_name", QVariant(QString, "OpenMandriva.html"))("filters", QVariant(QDBusArgument, ))("modal", QVariant(bool, true)))


filters etc. are all set correctly, but current_name is empty in the dialog.

Code looks correct though... Will debug a bit more.
Comment 18 Bernhard Rosenkränzer 2019-01-19 02:35:02 UTC
The dialog also doesn't respect current_folder even though the fileDialog->setDirectoryUrl code path is hit...
Starting to suspect a QFileDialog bug here, but haven't looked too closely yet
Comment 19 Bernhard Rosenkränzer 2019-01-19 02:51:49 UTC
The problem is that, at least here, with Qt 5.12.0, QFileDialog::setDirectoryUrl() expects a "real" URL and doesn't work if it's passed a QUrl with an empty scheme (such as QUrl(currentName) if currentName is a local file name).

Everything starts working correctly if I change the code to use QFileDialog::setDirectory() instead of QFileDialog::setDirectoryUrl(), or if I use QUrl("file:" + currentFolder) in place of QUrl(currentFolder).

Of course that will break things if currentFolder is remote, or is already a file: URL -- not sure if this can happen.
Will attach a patch with some safety checks, but if you know it's always a local directory name, one of the workarounds I've already mentioned is sufficient.
Comment 20 Bernhard Rosenkränzer 2019-01-19 03:16:08 UTC
Looks like this is fixed on git master by the switch from QFileDialog to KFileWidget; I've submitted a less intrusive patch to phabricator
https://phabricator.kde.org/D18378
in case anyone wants to put it on the 5.14 branch.
Comment 21 Nate Graham 2019-01-19 03:18:06 UTC
Thanks for the investigation, Bernhard. If it's already fixed in master, this bug should be closed, and I'm afraid there aren't any more 5.14.x releases scheduled. Each non-LTS plasma version gets 5, and 5.14.5 was already released. But you could target your patch for the 5.12 LTS branch maybe?