Replaces "Up" button with "Back" button and use history when going back.
Details
Details
- Reviewers
hein - Group Reviewers
Plasma - Commits
- R119:235d91109276: [Folder View] Replace Up button with Back button in listview mode
Diff Detail
Diff Detail
- Repository
- R119 Plasma Desktop
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
(a) I don't like accumulating data in a cache that's never evicted and never changed for correctness. What if the hierarchy changes in the meantime? This is like a subtle bug trap.
(b) It's "Up", not "Back". If there's no way up from a location then there's no way up. Maybe for links we should make it "Back", and just keep the previous URL around.
Comment Actions
The default behaviour is to treat desktop links as folder and for normal links dolphin (or the associated program) is launched.
It's just i only focused on desktop links. But this can be changed as well.
Comment Actions
I'm referring to links to folders outside of a parent. .desktop links to folders are sort of similar to symlinks.
Comment Actions
May be we can treat them as folders but then i guess we'll have to drop the "Up" button and introduce dolphin like behaviour.
To be honest I didn't bothered looking into this. So i can't say for sure.
Comment Actions
I don't follow you at all there, sorry - my original request to use Up normally and Back for links, and you ended up special-casing desktop for some reason with no explanation :)
Comment Actions
No, hardcoding magic behavior based on special URLs and inconsistent behavior depending on URL is not "simple".
Comment Actions
I "special cased" desktop because links were treated as folder only when folder view's location was set to "Desktop Folder".
Can you please elaborate "Up normally and Back for links". I am afraid i still don't get you.
Do you mean as we go down show "Up" but when a link (.desktop or symlink) to folder is clicked change directory but show "Back" , keep the previous dir's url around and carry on doing the same?
If so do you want this to be the case for Location="desktop:/" or for all locations?
Comment Actions
I "special cased" desktop because links were treated as folder only when folder view's location was set to "Desktop Folder".
Ah!
Actually, on further thought, I don't really having two different modes, and maybe having an "Up" in concert with the decision to follow links in general. How about we rename it to "Back" and keep a stack of previous URLs, put on the stack on every cd and make back pop it off? This seems more in line with what users probably want rather than ever jumping to a new hierarchy context and going "Up" or having the action change name.
Comment Actions
I'm not entirely happy with the way this works, I think the history may need to be kept on the QML side instead, then:
(a) You can properly evict the history when the URL in the config changes
(b) Tie the visibility of the Back button to whether the history is empty or not
So basically before QML runs FolderModel::cd write down the URL, then when back is pressed pop it off.
FolderModel can't know when to evict the history because it can't distinguish between setUrl coming from cd() or from a config change, only the applet code can.
Having history functionality in FolderModel is otherwise attractive, so this is a bit of a dilemma. Putting the history into QML however keeps FolderModel "dumber" in the sense that it doesn't have to reason about intent and just does what it's told, and the higher level situation is captured on the side of the user of the model instead.
| containments/desktop/plugins/folder/foldermodel.cpp | ||
|---|---|---|
| 487 | For future reference: This can be simplified to takeLast(). | |
Comment Actions
Added properties location(which will store folder view's location) and historyAvailable
Removed prop url and renamed prop resolvedUrl to currentUrl
Bind Back button's visibility to historyAvailable
For the logic; folder view must reset to location whenever plasmoid.expanded changes, added method goToLocation.
Comment Actions
I don't want two props in the model - I want it to remain simple and simply provide data for one location. It should be oblivious to the configuration concerns of its user. This is in line with KDirModel, too. Please implement in the suggested way.
Comment Actions
- Leave FolderModel::up unchanged
- There's no need for a FolderModel::back, it just duplicates setUrl
- I don't understand the need for updateHistory, please explain
Comment Actions
Push and pop methods were not updating the property might be they were working on another copy of "history". Due to this I used "history=history" so as to update it explicitly.
Comment Actions
Thanks for the explanation. Thoughts:
- Can you try { new Array() } or something instead of []? There's some confusion in QML between "QML list" and "JavaScript array", perhaps the prop notification works with an explicity array.
- If this doesn't work either, please add a code comment explaining why updateHistory exists.
- Perhaps instead of length = 0 assignments and calling updateHistory, just assigning a new Array would be cleaner?
Comment Actions
Looks like it will be the code comment route:
[17:38] <Sho_> btw
[17:39] <Sho_> i have a dev on phab who claims push/pop on a [] held in a var-type prop doesn't notify
[17:39] <Sho_> anyone run into this?
[17:39] <kbroulik> yup
[17:39] <kbroulik> that's by design™
[17:39] <kbroulik> QML can't detect that
[17:39] <Sho_> even for a JS array?
[17:39] <kbroulik> yup
[17:39] <Sho_> or is this just with QML lists
[17:39] <kbroulik> also for JS objects
[17:39] <Sho_> ok ...
[17:39] <Sho_> what did you do to work around?
[17:39] <kbroulik> var foo = prop
[17:39] <kbroulik> foo.push(bar)
[17:39] <kbroulik> prop = foo
[17:40] <Sho_> how aful