Additional patch for Bug 362474 to make the targetUrl persistent again.
Save temporarily stored path of Copy To/Move To file dialogues from ContextManager in rc-file and restore on next start.
CCBUG: 362474
Details
Details
- Reviewers
rkflx - Group Reviewers
KDE Applications
Diff Detail
Diff Detail
- Repository
- R260 Gwenview
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
Thanks, that was fast. Upload of the patch and repo seem fine so far. I'll try to review in the next couple of days. In case we land this, we would need your email address (if it should be different to the one you use on bugzilla).
Nothing you could've known, but if you add CCBUG: 362474 as a separate line to the summary (which will become the commit message), the bug will get notified on commit.
Comment Actions
Gave this a try, in some cases it works but I could also break it:
- Select two images, Copy To.
- (Optional: restart app), repeat previous step.
- Observe how instead of the previous directory a different directory is shown.
Turns out this is not a problem with your patch but already present in current master, so likely a deficiency of 7be60997b747. Unfortunately this prevents me from testing your patch properly… This probably is the cause for storing the complete filename when operating on a single file and only the directory when selecting multiple files. Checking the KDE4 version of Gwenview, only the directory was stored in both cases (makes sense, because the filename should always come from the new file and not from the old config).
My second observation is that you store the complete path (e.g. file:///home/user/Pictures), while the rest of the entries in the config file uses a shorter form (e.g. file:$HOME/Pictures). Not sure what's causing this, but you could try to figure it out. Note that this should still work with remote targets like fish:/ (it does in KDE4).
In summary:
- We need to fix the bug somehow. @sthiel Still around? Could you help here?
- Storing the directory only might come for free by fixing the bug, but could you maybe rename to LastTargetDir?
- Try to investigate how to get similar URLs to the already existing ones.
- Would it be feasible to adapt the code organization a little bit to the existing style (see below)?
(I'm setting this now to "Request Changes", please don't get too discouraged by the red icons ;)
| app/mainwindow.cpp | ||
|---|---|---|
| 1441 | Looking at the lines above, I wonder whether we should just add ContextManager::loadConfig()? | |
| 1455 | ContextManager::saveConfig()? Note how GwenviewConfig:: is used in the functions above, see also lib/gwenviewconfig.kcfgc. Apparently this is using KConfig XT. | |
Comment Actions
Yes, the problem is the storage of a file url instead of a path in ContextManager. I noticed this, but did not discover any effects.
I'll try to fix it.
(Thanks for the hints - I'm not discouraged, it's just another challenge. ;) )
Comment Actions
[..]
In summary:
- We need to fix the bug somehow. @sthiel Still around? Could you help here?
- Storing the directory only might come for free by fixing the bug, but could you maybe rename to LastTargetDir?
- Try to investigate how to get similar URLs to the already existing ones.
- Would it be feasible to adapt the code organization a little bit to the existing style (see below)?
(I'm setting this now to "Request Changes", please don't get too discouraged by the red icons ;)
Hi,
sorry, I was quite busy within the last months.
If it is still any help, I could try to have a look in the next days.
Just for the context: The original reason for the bug was the porting to QFileDialog:
https://bugs.kde.org/show_bug.cgi?id=362474#c7
Regards
Simon
Comment Actions
That's great. It doesn't really matter in the end who's fixing the bug :)
I'd suggest to just open a new Diff for that, whoever gets around to it first. Once the bug is fixed, we'll continue with persisting the path in the config file over here.
Comment Actions
I have created a new patch:
https://phabricator.kde.org/D8785
I think, this is done by the KDE versions of some classes. The old KFileDialog is deprecated and QFileDialog does not automatically convert this.
Network protocol fish:// works ok and is stored in rc-file as LastTargetDir.
I'm not sure who I should add as reviewers to the new diff. Is it ok, just to add someone (e.g. sthiel) or should I ask them before?
Comment Actions
Ideally you'd add the Phabricator project (Gwenview) or the maintainer of the application as reviewer. Unfortunately, Gwenview does not have either ATM. Looking at the commit history who was recently active would be the next thing to check. No need to ask, just don't spam people with stuff they know nothing about or are not interested in. IOW, I'll review it in the next days (busy currently, sorry) ;)
Comment Actions
As this Diff is superseded by D8785, please "Abandon" via "Add Action" at the bottom.