Konsole not save last output save directory.
User need choose the directory every time
Signed-off-by: Tao Guo <guotao945@gmail.com>
BUG: 403103
FIXED-IN: 19.08.0
Details
Details
- Reviewers
- None
- Group Reviewers
Konsole - Commits
- R319:61ed2c88a4e2: Remember last 'Save Output As' directory
Save output to a directory except home dir
Save output again and check the dir
Diff Detail
Diff Detail
- Repository
- R319 Konsole
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 10186 Build 10204: arc lint + arc unit
| src/SaveHistoryTask.cpp | ||
|---|---|---|
| 81 | url is a file, not current directory. dialog->directoryUrl() should work | |
Comment Actions
This works fine during a single Konsole session, but the output directory is not remembered across application launches; when you quit and restart Konsole, the default save directory is ~ again. In Spectacle, the output directory is remembered across launches. What do you think?
Comment Actions
@ngraham
I think it's no necessary remember the directory across different launches.
- User want save log to same directory for two different sessions, they can open two tabs in konsole
- User want save log to different directories for two sessions, they can launch two konsole
| src/SaveHistoryTask.cpp | ||
|---|---|---|
| 81 | @mglb
For 1,2 file is better For me, I use case 2 most time during working. | |
Comment Actions
In case of using file URL, I would change the variable name (like _saveDialogFileURL or something) and use selectURL() after creating the dialog object. The constructor, according to documentation, takes URL to the directory. It works OK now, but in theory can change in the future.
When using selectURL(), home directory could be always used in the constructor, and the function would be called only if _saveDialogFileURL is not empty.
| src/SaveHistoryTask.cpp | ||
|---|---|---|
| 81 | Good idea. It asks for overwrite confirmation, the file name is already selected (so typing another name takes as much work as when there were no name). No downsides. | |
Comment Actions
Thanks, this works but I think there might be a better way of doing this - let me work on it - if I can't find a better way I'll merge this
Comment Actions
@hindenburg, Thank for your help to make it better.
Actually I tried to use URL instead before, but made the code more complex, so I gave up.
Comment Actions
This was what I was thinking about - it seems to work - let me know if you have concerns.
diff --git a/src/SaveHistoryTask.cpp b/src/SaveHistoryTask.cpp index a202914d..4f31e3a8 100644 --- a/src/SaveHistoryTask.cpp +++ b/src/SaveHistoryTask.cpp @@ -26,6 +26,9 @@ #include <KMessageBox> #include <KLocalizedString> +#include <KSharedConfig> +#include <KConfig> +#include <KConfigGroup> #include "SessionManager.h" #include "Emulation.h" @@ -45,9 +48,7 @@ void SaveHistoryTask::execute() // three then providing a URL for each one will be tedious // TODO - show a warning ( preferably passive ) if saving the history output fails - QFileDialog* dialog = new QFileDialog(QApplication::activeWindow(), - QString(), - QDir::homePath()); + QFileDialog* dialog = new QFileDialog(QApplication::activeWindow()); dialog->setAcceptMode(QFileDialog::AcceptSave); QStringList mimeTypes { @@ -56,6 +57,15 @@ void SaveHistoryTask::execute() }; dialog->setMimeTypeFilters(mimeTypes); + KSharedConfigPtr konsoleConfig = KSharedConfig::openConfig(); + const auto group = konsoleConfig->group("KFileDialog Settings"); + const auto list = group.readPathEntry("Recent URLs", QStringList()); + if (list.isEmpty()) { + dialog->setDirectory(QDir::homePath()); + } else { + dialog->setDirectoryUrl(QUrl(list.at(0))); + } + // iterate over each session in the task and display a dialog to allow the user to choose where // to save that session's history. // then start a KIO job to transfer the data from the history to the chosen URL
Comment Actions
@hindenburg why is URL a list when only one entry is used? With string type homePath() could be used in readPathEntry as default value, making if...else... redundant.
Comment Actions
If you look in ~/.config/konsolerc
[KFileDialog Settings] Recent Files[$e]=file:///tmp/bb.txt Recent URLs[$e]=file:///tmp/
homePath() = "/home/kurthindenburg"
So the Recent URL has "file://" while homePath does not; also there could be many Recent entries according to spec
Comment Actions
Sorry, I've missed Url in setDirectoryUrl :/
This part in config seemed strange, as Konsole does not write it. I've looked where it comes from:
https://github.com/KDE/kio/blob/master/src/filewidgets/kfilewidget.cpp#L2037
The config is also read in this file - it populates file and path combo boxes in the dialog. The KFileWidget is used in KDE Plasma Platform Theme to implement QFileDialog.
Therefore "Recent URLs" is KDE-specific - it won't be updated on other platforms.
You can test it with GTK platform theme: XDG_CURRENT_DESKTOP=GNOME konsole (in Kubuntu qt5-gtk-platformtheme is needed) or Qt5 built-in platform theme: XDG_CURRENT_DESKTOP=invalid konsole
Comment Actions
The other option I looked is using KFileWidget and KRecents but there's issues there. Ping me if it gets close to July w/o a resolution here.
QUrl fileUrl = KFileWidget::getStartUrl(QUrl(QStringLiteral("kfiledialog:///SaveHistoryUrl")), recentDirClass);
KRecentDirs::add(recentDirClass, url.adjusted(QUrl::RemoveFilename|QUrl::StripTrailingSlash).toString());Comment Actions
Well I decided to try again. This seems much cleaner and doesn't add any extra dependents. It works locally and with sftp URLs. On a side note, I don't think the foreach() loop actually works, so there should be real issue w/ reading/writing the config file every time.
diff --git a/src/SaveHistoryTask.cpp b/src/SaveHistoryTask.cpp index a202914d..54156313 100644 --- a/src/SaveHistoryTask.cpp +++ b/src/SaveHistoryTask.cpp @@ -26,6 +26,9 @@ #include <KMessageBox> #include <KLocalizedString> +#include <KSharedConfig> +#include <KConfig> +#include <KConfigGroup> #include "SessionManager.h" #include "Emulation.h" @@ -45,9 +48,7 @@ void SaveHistoryTask::execute() // three then providing a URL for each one will be tedious // TODO - show a warning ( preferably passive ) if saving the history output fails - QFileDialog* dialog = new QFileDialog(QApplication::activeWindow(), - QString(), - QDir::homePath()); + QFileDialog* dialog = new QFileDialog(QApplication::activeWindow()); dialog->setAcceptMode(QFileDialog::AcceptSave); QStringList mimeTypes { @@ -56,6 +57,15 @@ void SaveHistoryTask::execute() }; dialog->setMimeTypeFilters(mimeTypes); + KSharedConfigPtr konsoleConfig = KSharedConfig::openConfig(); + auto group = konsoleConfig->group("SaveHistory Settings"); + const auto list = group.readPathEntry("Recent URLs", QStringList()); + if (list.isEmpty()) { + dialog->setDirectory(QDir::homePath()); + } else { + dialog->setDirectoryUrl(QUrl(list.at(0))); + } + // iterate over each session in the task and display a dialog to allow the user to choose where // to save that session's history. // then start a KIO job to transfer the data from the history to the chosen URL @@ -76,6 +86,9 @@ void SaveHistoryTask::execute() continue; } + // Save selected URL for next time + group.writePathEntry("Recent URLs", url.adjusted(QUrl::RemoveFilename|QUrl::StripTrailingSlash).toString()); + KIO::TransferJob* job = KIO::put(url, -1, // no special permissions // overwrite existing files
Comment Actions
It seems not isolated for different konsole instances.
Usually I have two konsole instance to save log files. one for serials com, one for adb log.
Comment Actions
Ok that would require each tab to save its previous save location. Which is what your patch did. I was thinking more of saving the previous save location across all tabs and upon next konsole startup.
Comment Actions
Can you try this?
diff --git a/src/SaveHistoryTask.cpp b/src/SaveHistoryTask.cpp index a202914d..16ab661c 100644 --- a/src/SaveHistoryTask.cpp +++ b/src/SaveHistoryTask.cpp @@ -26,12 +26,17 @@ #include <KMessageBox> #include <KLocalizedString> +#include <KSharedConfig> +#include <KConfig> +#include <KConfigGroup> #include "SessionManager.h" #include "Emulation.h" namespace Konsole { +QString SaveHistoryTask::_saveDialogRecentURL; + SaveHistoryTask::SaveHistoryTask(QObject* parent) : SessionTask(parent) { @@ -45,9 +50,7 @@ void SaveHistoryTask::execute() // three then providing a URL for each one will be tedious // TODO - show a warning ( preferably passive ) if saving the history output fails - QFileDialog* dialog = new QFileDialog(QApplication::activeWindow(), - QString(), - QDir::homePath()); + QFileDialog* dialog = new QFileDialog(QApplication::activeWindow()); dialog->setAcceptMode(QFileDialog::AcceptSave); QStringList mimeTypes { @@ -56,6 +59,20 @@ void SaveHistoryTask::execute() }; dialog->setMimeTypeFilters(mimeTypes); + KSharedConfigPtr konsoleConfig = KSharedConfig::openConfig(); + auto group = konsoleConfig->group("SaveHistory Settings"); + + if (_saveDialogRecentURL.isEmpty()) { + const auto list = group.readPathEntry("Recent URLs", QStringList()); + if (list.isEmpty()) { + dialog->setDirectory(QDir::homePath()); + } else { + dialog->setDirectoryUrl(QUrl(list.at(0))); + } + } else { + dialog->setDirectoryUrl(QUrl(_saveDialogRecentURL)); + } + // iterate over each session in the task and display a dialog to allow the user to choose where // to save that session's history. // then start a KIO job to transfer the data from the history to the chosen URL @@ -76,6 +93,10 @@ void SaveHistoryTask::execute() continue; } + // Save selected URL for next time + _saveDialogRecentURL = url.adjusted(QUrl::RemoveFilename|QUrl::StripTrailingSlash).toString(); + group.writePathEntry("Recent URLs", _saveDialogRecentURL); + KIO::TransferJob* job = KIO::put(url, -1, // no special permissions // overwrite existing files diff --git a/src/SaveHistoryTask.h b/src/SaveHistoryTask.h index c90c7e9e..65504d6f 100644 --- a/src/SaveHistoryTask.h +++ b/src/SaveHistoryTask.h @@ -68,6 +68,8 @@ private: }; QHash<KJob *, SaveJob> _jobSession; + + static QString _saveDialogRecentURL; }; }