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
rkflx |
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
Automatic diff as part of commit; lint not applicable. |
Automatic diff as part of commit; unit tests not applicable. |
(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.)
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… |
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:
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 | String → Path, 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… |
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. ;)
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? |
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. |
Ok, let's go. :)
lib/gwenviewconfig.kcfg | ||
---|---|---|
50 | If I try to convert "/home/user" to a QUrl, I get an error: |
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. |
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?) |
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> ? |
Thanks for the tips, Elvis.
I looked around a bit, but could not find anything regarding:
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):
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.