Enable option to show hidden folders in sidebar, plus small sidebar code fixes (eg: scrollbar display)
AcceptedPublic

Authored by rrosch on Jul 13 2020, 3:18 PM.

Details

Reviewers
dfaure
Summary

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.

Test Plan

-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.

Diff Detail

Repository
R226 Konqueror
Branch
konqsidebar_tree_module_PHAB (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27189
Build 27207: arc lint + arc unit
rrosch requested review of this revision.Jul 13 2020, 3:18 PM
rrosch created this revision.
rrosch retitled this revision from Enable option to hidden folders in sidebar, plus small sidebar code fixes (eg: scrollbar display) to Enable option to show hidden folders in sidebar, plus small sidebar code fixes (eg: scrollbar display).Jul 13 2020, 3:21 PM

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 :-)

rrosch added a comment.EditedTue, Jul 14, 1:13 PM

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).

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?

dfaure requested changes to this revision.Sat, Jul 18, 7:47 PM

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.

This revision now requires changes to proceed.Sat, Jul 18, 7:47 PM

BTW we have moved to gitlab for merge requests :-)

Argh. So how do I do it now?

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).

I can't really approve a half finished feature that is undiscoverable.

rrosch updated this revision to Diff 83333.Wed, Jul 29, 7:48 AM

Added the context menu code.

Restricted Application added a project: Documentation. · View Herald TranscriptWed, Jul 29, 7:48 AM
Restricted Application added a subscriber: kde-doc-english. · View Herald Transcript
rrosch edited the summary of this revision. (Show Details)Wed, Jul 29, 7:53 AM
rrosch edited the test plan for this revision. (Show Details)
dfaure added inline comments.Sat, Aug 1, 8:26 PM
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);

541

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.
And since it's available for any user of KDirModel, I'd just make it depend on the module being used.

578

"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.

rrosch added inline comments.Wed, Aug 5, 3:52 AM
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?

541

Sure, I can make it available for any KonqSideBarTreeModule button if you want. Will do in the next revision.

578

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.

rrosch added inline comments.Wed, Aug 5, 4:22 AM
doc/konqueror/index.docbook
2141

Actually, konqueror is compiled for Windows too, right? Does KDirModel make that distinction in "setShowingDotFiles"?

rrosch updated this revision to Diff 83339.Wed, Aug 5, 4:28 AM

Added changes as requested.

rrosch updated this revision to Diff 83340.Wed, Aug 5, 4:32 AM
rrosch marked an inline comment as done.

Removing unnecessary comment.

rrosch marked 3 inline comments as done.Wed, Aug 5, 4:33 AM
dfaure added inline comments.Wed, Aug 5, 3:47 PM
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.

578

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.
I doubt Dolphin does that though?

dfaure added inline comments.Wed, Aug 5, 3:50 PM
sidebar/sidebar_widget.cpp
577

The naming m_ (followed by lowercase, BTW) is for member variables.

Please s/m_T/t/ and join with the next line.

rrosch updated this revision to Diff 83341.Wed, Aug 5, 7:29 PM

Latest round of modifications according to review notes.

dfaure added a comment.Wed, Aug 5, 8:18 PM

A bunch of fixes got reverted. The dead code is back. The assert is gone. The docu improvement is gone....

rrosch added a comment.Wed, Aug 5, 8:54 PM

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.

dfaure added a comment.Thu, Aug 6, 8:31 AM

I didn't have an assert before though, should I replace the if statement with a Q_ASSERT then?

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".

rrosch updated this revision to Diff 83345.Mon, Aug 10, 9:14 PM

Q_ASSERT instead of redundant if(), as suggested

I didn't have an assert before though, should I replace the if statement with a Q_ASSERT then?

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.

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.

dfaure accepted this revision.Tue, Aug 11, 8:33 AM
This revision is now accepted and ready to land.Tue, Aug 11, 8:33 AM