Persist path for "Copy/Move/Link To" to config file
ClosedPublic

Authored by muhlenpfordt on Nov 13 2017, 11:09 AM.

Details

Summary

Additional patch to make the targetUrl persistent again.
Modified storage of dirpath only instead of filepath.
Save temporarily stored path of Copy/Move/Link To file dialogues from ContextManager in rc-file and restore on next start.

CCBUG: 362474

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
muhlenpfordt created this revision.Nov 13 2017, 11:09 AM
rkflx added a subscriber: rkflx.EditedNov 13 2017, 11:40 AM

(Ah, you now have both the bug fix and the persistence feature in one patch. In that case, you could've just updated the other Diff (D8747). Anyway, will look at it in the next days.)

rkflx added a subscriber: dhaumann.EditedNov 15 2017, 6:57 PM

Will do a proper review in a bit, got a question to clear up first…

@dhaumann Hi from (maintainerless) Gwenview land, could you help us out here? (see below)

(edit: Cannot modify inline comments in Phab, I mean SC of course :)

lib/contextmanager.h
77

@dhaumann In case we have to maintain BC, we cannot change the function name here. Could you tell us where to look or what to look for to determine whether a piece of code or even a repository is under a BC promise? This is not a Framework, but an Application, still has lib in the name and uses CMake's GenerateExportHeader, which makes me wonder…

rkflx requested changes to this revision.Nov 17 2017, 5:18 PM

Great work on the additional bug fix and the code reorganization. We are getting there, only some minor things left to improve:

The title of this Diff will become the title of the commit message. Therefore I'd prefer Persist path for "Copy To/Move To" to config file or something similar, i.e. it should not sound like a bug report but refer to the new behaviour or that something was fixed.

Regarding the potential SC breakage, we have two options:

  • Roll back the renaming now. Once we have a proper answer and it is no problem, do the renaming in a later patch.
  • Wait until we have a proper answer, commit only then either with or without renaming.

Personally I don't mind, as long as the patch gets in before the RC on November 30.

Please also see the inline comments below. You should be able to upload a new version of the patch by clicking on "Update Diff". Thanks for bearing with this somewhat lengthy process for a "simple" patch attached to Bugzilla, it gets easier after the first time ;)

lib/contextmanager.cpp
140

When the user never used Copy/Move To before, the config entry does not exist yet, leading to an empty URL. setTargetDirUrl will then complain (rightly so) with:

Condition ' url.isValid() ' failed

To fix, just add this to the kcfg entry:

<default code="true">
  QStandardPaths::writableLocation(QStandardPaths::PicturesLocation)
</default>

I got the idea when reading this and adapted it slightly.

lib/gwenviewconfig.kcfg
48

StringPath, this way we get $HOME written to the config file, which comes in handy when regularly moving between machines with different paths.

How did I find this? Well, I just glanced over the tutorial I linked in the other Diff and learned something new myself…

This revision now requires changes to proceed.Nov 17 2017, 5:18 PM
muhlenpfordt retitled this revision from Bug 362474 - Copy To/Move To does not remember path any more - Modified persistance patch to Persist path for "Copy/Move/Link To" to config file.Nov 20 2017, 8:58 AM
muhlenpfordt edited the summary of this revision. (Show Details)
muhlenpfordt added a comment.EditedNov 20 2017, 9:04 AM
In D8785#169082, @rkflx wrote:

Regarding the potential SC breakage, we have two options:

  • Roll back the renaming now. Once we have a proper answer and it is no problem, do the renaming in a later patch.
  • Wait until we have a proper answer, commit only then either with or without renaming.

I returned to the original names, this seems to be the safest way for now.

Please also see the inline comments below.

This kcfg file code is cool - never noticed this before. ;)

muhlenpfordt added inline comments.Nov 20 2017, 9:16 AM
lib/gwenviewconfig.kcfg
50

I convert to QUrl and back to QString to get a complete url, e.g. file:///home/user instead of /home/user. Is there any easier way?

rkflx accepted this revision.Nov 20 2017, 4:06 PM

Patch looks good to me now. Let me know if you are happy with the current state too, then I'll commit to the stable branch on your behalf.

lib/gwenviewconfig.kcfg
50

Interesting observation, but why would you actually need file://? As far as I can see, my original suggestion (which directly returns a string) also leads to a valid URL via QUrl(...) later on. I'm not sure what difference this makes, as long as we don't ask for something like scheme().

At least on XDG compliant environments (probably boils down to this mechanism) I would not expect remote protocols, also the docs of QStandardPaths::writableLocation only talk about "directories".

On the other hand, the importer code does it this way and the approach is more portable in case of future changes, of course. So I think your current solution is just fine and does not need changing.

This revision is now accepted and ready to land.Nov 20 2017, 4:06 PM
In D8785#170112, @rkflx wrote:

Patch looks good to me now. Let me know if you are happy with the current state too, then I'll commit to the stable branch on your behalf.

Ok, let's go. :)

lib/gwenviewconfig.kcfg
50

If I try to convert "/home/user" to a QUrl, I get an error:
kf5.kio.core: Invalid URL: QUrl("/home/user")
The only way I found, is QUrl::fromLocalFile to do this.

This revision was automatically updated to reflect the committed changes.
rkflx added a comment.Nov 20 2017, 4:55 PM

Thanks again for submitting and polishing the patch. Let me know if you are interested in further contributions and need ideas to work on. There are lots of easy (and also more complicated) bugs in Gwenview…

(And please abandon the other Diff.)

lib/gwenviewconfig.kcfg
50

Just tried it again, you are right. My testing was flawed, i.e. I started Gwenview in "~/Pictures", so I did not notice the problem. The URL is valid, but KIO stumbles.

Learned something, yeah! Thanks for finding and fixing the problem ;)

@elvisangelaccio Hi from (maintainerless) Gwenview land, could you help us out here with a question we have? It's probably the same in Dolphin, and I really want to know this properly for the future…

D8785#inline-39103 ← question and context here…

lib/contextmanager.h
77

In general you should maintain BC if the library is "public", i.e. if someone else is using this code outside gwenview. The fact that this code is "exported" doesn't mean anything in particular, if the library is private you don't really care about BC.

rkflx added inline comments.Nov 20 2017, 5:47 PM
lib/contextmanager.h
77

Thanks, good to know. Is there a particular expression to search for on lxr.kde.org to determine whether someone outside of Gwenview is using this?

(The class uses d-pointers, so I guess there was at least some intention to promise SC/BC at some point?)

rkflx added inline comments.Nov 20 2017, 5:48 PM
lib/gwenviewconfig.kcfg
50

…I can search for the class name, of course. (Sorry for the noise).

lib/contextmanager.h
77

Try to look for the cmake bits (e.g. whatever you would need to pass to the target_link_libraries() call).

Or maybe just for #include <contextmanager.h> ?

rkflx added a comment.Dec 6 2017, 6:11 PM

Thanks for the tips, Elvis.

I looked around a bit, but could not find anything regarding:

  • Other users of Gwenview's code (searched lxr for gwenviewlib and gvpart).
  • Promises we make about libraries shipped by applications (in contrast to Frameworks).

Of course, not finding anything might just mean the method of search was flawed or the code is not public. Next, I tried to find precedences where we broke BC before and looked for complaints (could not find any):

  • 3e10699ac37c added a parameter to setActions, but "extending a function with another parameter, even if this parameter has a default argument" is not allowed
  • 2909dc665708 modified mAnim, but we cannot "change the type of the member"

To conclude, there is nothing really which explicitly says we can break BC, but all circumstantial evidence leads to believe we could just risk it. Maybe the d-pointers and exported headers were introduced in the past with the intention to do more with lib/* in the future or served a different purpose, but for now I'd say we should not worry too much.

If anyone knows better, please let me know.