| dfaure |
Adds option to use the "ShowHiddenFolders" option in .desktop files for sidebar buttons, as well as a context menu entry to toggle it when the option is present in the .desktop file.
Included is a small fix for the width of the column display, which now allows for the scrollbar to be displayed properly when the tree does not fit the horizontal area. This is still not perfect though.
It also fixes the "const" keyword having been put in the wrong place.
-Edit your version of root.desktop (or create a new .desktop sidebar entry) to set "ShowHiddenFolders=true" and relaunch konqueror. Note the appearance of hidden folders in the sidebar for root, (but not other entries).
-Right click on the buttons for "Home" and "Root" and note the appearance of new entries to "show" (or "hide") hidden folders. This entry only appears when the .desktop file has the "ShowHiddenFolders" entry (no matter the value).
-Expand the sidebar tree and note that the horizontal scrollbar only appears when the contents do not fit, including the scrollbar not displaying when they do fit in the current area. Resize the sidebar to confirm.
| No Linters Available |
| No Unit Test Coverage |
| Buildable 27185 | |
| Build 27203: arc lint + arc unit |
The commit log explains the what but not the why. Can you give more details about the use case for this? At first sight it smells like overconfigurability (given that the user can already request that hidden files are shown, more globally).
BTW we have moved to gitlab for merge requests :-)
Hidden files and folders show up in the view area, but not the sidebar. So while you can click on the view to navigate to hidden folders, the sidebar will not show them. This is in general a good thing, since hidden folders can clutter up the sidebar, but sometimes you will want convenient navigability for hidden folders in the sidebar too. This just makes it possible without having to compile (so the user can either edit their .desktop file or create a new one for this purpose).
BTW we have moved to gitlab for merge requests :-)
Argh. So how do I do it now?
Users don't create or edit desktop files ;)
Why not do this at runtime?
You go into a hidden directory, boom, the sidebar shows hidden directories, so that it can show the current one.
You go back to a normal directory, boom, it switches off again.
| sidebar/default_entries/home.desktop | ||
|---|---|---|
| 6 | Not needed, it's the default value. | |
https://community.kde.org/Infrastructure/GitLab
But we can finish this one here if you prefer.
Ahh, that would definitely be an interesting approach. Would you be opposed to having what is currently done incorporated now? I can't allocate time to implement your approach, or think of use cases where someone might want to choose one over the other (or potential problems with your approach), but at least with this we can have the option in there. For sure a context menu toggle would avoid any potential issues there could be (I have tons of old hidden folders in my home directory for example, I wouldn't always want it showing or toggling automatically on).
I included the line with the default value as a hint that this can be changed to anyone poking around in there (it's actually how I tend to find a lot of features.. especially undocumented ones).
At some point the "add new button" should be implemented, and from what I could see is already in there, this option could easily be toggled for new additions too.
Well, if you insist, but this is how cruft just adds up. Will you -- or someone else -- remember to actually remove this weird entry from desktop files when implementing something better?
I just don't see how it belongs there. To make this actually available to users, you'd have to ship two desktop files "Home" and "Home with hidden directories shown"? Urgh.
I agree about the cruft, and I highly recommend against shipping two home desktop files. As I have it now, it's just one extra line in two files, which an administrator could edit, or copy, if so motivated. At least on FC30, the kio plugin for fonts:/ is included in a package separate to konqueror (plasma_desktop, if I remember correctly), so someone could theoretically just create packages for sidebar buttons they want to have on their system, and use the current button .desktop files for reference. It's what falkon is doing for themes, for example.
Since the sidebar is a feature I am highly interested in, it's something I keep going back to from time to time, tweaking little things, and at some point I might have the time and motivation to add the context menu item and will edit the desktop files to remove that from there. Or, if someone else takes it up, they could do it too (so, should we maybe add a comment in the cpp file as a reminder for that? I already have the TODO list in there, we could add the note there).
| doc/konqueror/index.docbook | ||
|---|---|---|
| 2141 | hidden folders (whose name starts with a dot) That's what this is all about, right? | |
| sidebar/sidebar_widget.cpp | ||
| 339 | This should be an assert instead, you don't create the action if canToggle... is false. Q_ASSERT(currentButtonInfo().canToggleShowHiddenFolders); | |
| 542 | Why not enable the feature for *any* button that ends up creating a KonqSideBarTreeModule? Otherwise, as a user, I do "RMB / Add New / Tree Sidebar Module", configure its URL to /tmp or whatever and the feature is broken there. By not making the on/off dependent on the desktop file entry, maybe one day we can change the storing of the on/off status to be per-directory. Availability of the feature and on/off storage should be separate. | |
| 579 | "Hide Hidden" feels weird. Please copy Dolphin. It uses a toggle action "Show Hidden Folders", the text doesn't change, just the checkmark in front. QWidget is all single threaded, I wonder which race condition you have in mind here, please explain. | |
| sidebar/tree_module/tree_module.cpp | ||
| 76 | Please don't add dead code. | |
| doc/konqueror/index.docbook | ||
|---|---|---|
| 2141 | Do you mean that I should edit the text to add the stuff in the parentheses? | |
| sidebar/sidebar_widget.cpp | ||
| 339 | I haven't used Q_ASSERT before, and when I looked it up just now it says that all it does is print a warning message if the boolean is false. Is that what you mean? | |
| 542 | Sure, I can make it available for any KonqSideBarTreeModule button if you want. Will do in the next revision. | |
| 579 | Ok, I'm going to look up how to do the checkbox. Hopefully it's straightforward and doesn't require too much code. The race condition I had in mind was when you have two windows open and you change the toggle in one, and then again in the other. Stefano says that his Konq does the change live, but that doesn't seem to be working for me (not sure if my Konq, KF5 or Qt is to blame), but in any case if the change happens live, then that solves it and I guess there is no race condition. | |
| sidebar/tree_module/tree_module.cpp | ||
| 76 | Yeah this was by mistake, I somehow missed it while doing code cleanup and only realized it was still in after I had already submitted the new commit. | |
| doc/konqueror/index.docbook | ||
|---|---|---|
| 2141 | Actually, konqueror is compiled for Windows too, right? Does KDirModel make that distinction in "setShowingDotFiles"? | |
| doc/konqueror/index.docbook | ||
|---|---|---|
| 2141 | Yes, this was a suggestion for improving the docu. | |
| 2141 | You know it's opensource, right? ;-) KCoreDirLister uses it to check KFileItem::isHidden() which is says "starts with dot = hidden" (on all platforms) and also supports kioslaves that set UDS_HIDDEN (lxr says smb:, fonts:, and desktop: for .desktop files that shouldn't be shown in KDE or due to TryExec). OK so it's technically more than "starts with dot", though for the common case of local files, that's all it's about indeed. | |
| sidebar/sidebar_widget.cpp | ||
| 339 | A failed assertion aborts the program, in debug mode. It's a way for developers to catch logic errors. | |
| 579 | OK, this isn't what I call a race condition (because it can be triggered very very slowly, that would be a snails race...). I call this a synchronization issue. At least konqueror is a unique service (single process) these days so it's easier than before, no DBus needed. If you have multiple windows open, I don't see how checking the action in one would immediately affect the other, you would have to implement that. | |
| sidebar/sidebar_widget.cpp | ||
|---|---|---|
| 578 | The naming m_ (followed by lowercase, BTW) is for member variables. Please s/m_T/t/ and join with the next line. | |
A bunch of fixes got reverted. The dead code is back. The assert is gone. The docu improvement is gone....
Argh, sorry about that.
I didn't have an assert before though, should I replace the if statement with a Q_ASSERT then? I thought that was just to catch errors, but since this is just an option (and hence shouldn't cause problems when false) it felt strange to do it that way.
For the doc, I thought you were saying that since in Windows you have hidden folders without the dot that it was ok to remove that part.
I'll wait for your confirmation on those things this time before updating the commit.
Yes I still believe you should.
I thought that was just to catch errors
Yes, it is. If everything works as intended, this method will never be called with canToggleShowHiddenFolders == false.
And if() makes people think that it can, which is just not true.
but since this is just an option
What is an option?
(and hence shouldn't cause problems when false)
Confusing code is a problem.
For the doc, I thought you were saying that since in Windows you have hidden folders without the dot that it was ok to remove that part.
I don't see where I said that. On the contrary, I said this was working the same on all platforms, Windows included.
There are some special cases for 3 protocols, but that's too much detail. Still the 99% rule is "starts with a dot".
Ohhh.. now I see what you mean, I had mentally lost track of some of the revision changes, and forgot that the option to show the context menu option was already being selected in a previous part of the code (as opposed to here).
I have now implemented the changes you suggested. Hopefully it's all ok now.
This is specifically useful because it enables the user to drag those entries into the sidebar buttons area to easily create shortcut buttons.
Trying to roll back the diff made to this revision instead of having created a new revision.
Ah, I forgot I had to land your patches manually.
I'll do it now (already resolving a few conflicts...)
Please please move to gitlab ;-)