Show partial path in Tabswitcher Ctrl+Tab list to distinguish equally named files
ClosedPublic

Authored by gregormi on Oct 8 2018, 9:31 PM.

Details

Summary

Currently, when there are equally named files open, e.g. some README.md files, then with Ctrl+Tab it is not possible to find the correct one instantly.

Screenshot BEFORE:

This chnage extends the Ctrl+Tab list with a new column to the left of the filename. It contains the partial paths (the common prefix of the strings are stripped away to avoid to show redundant information) to the file in grey color.

Screenshot AFTER:

If the user bothers to use the mouse, a tooltip can be revealed which shows the full path:

ISSUES:

  • Auto-sizing of the columns does not work yet (see the first file which is located in "asynckit" but only "asyn" is displayed. Strange. I havn't figured out how to handle this properly.
Test Plan
  • Test with documents that have no associated file: Works: no file path is shown:

  • Add a file with a different base directory and see how the prefix changes accordingly (this is also the special case where the prefix is only of length 1 and is therefore kept):

Diff Detail

Repository
R40 Kate
Branch
my_tabswitch
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3869
Build 3887: arc lint + arc unit
gregormi created this revision.Oct 8 2018, 9:31 PM
Restricted Application added a project: Kate. · View Herald TranscriptOct 8 2018, 9:31 PM
Restricted Application added a subscriber: kwrite-devel. · View Herald Transcript
gregormi requested review of this revision.Oct 8 2018, 9:31 PM
gregormi retitled this revision from poc to POC: Show partial path in Tabswitcher Ctrl+Tab list to better identify files.Oct 8 2018, 9:39 PM
gregormi edited the summary of this revision. (Show Details)
ngraham added a subscriber: ngraham.

Very cool. For comparison's sake, here's how it's handled on macOS:

gregormi added a comment.EditedOct 8 2018, 9:52 PM

Hi Nate, you review faster than I can fill out the Reviewer field :-).

Thanks for the MacOS comparison screenshot. At first glance, it looks a bit untidy to me *g* (though this kind of stuff is hard to make look clean).

This is how the Tabswitcher dialog in KDevelop looks like:

kate/katequickopen.cpp
190–197 ↗(On Diff #43171)

Unrelated for this POC. This tries to remove the file:/// prefix of open files compared to non-open files:

Or is this intended?

gregormi edited the summary of this revision. (Show Details)Oct 8 2018, 10:00 PM
gregormi edited the test plan for this revision. (Show Details)
gregormi edited the summary of this revision. (Show Details)Oct 8 2018, 10:03 PM
gregormi retitled this revision from POC: Show partial path in Tabswitcher Ctrl+Tab list to better identify files to POC: Show partial path in Tabswitcher Ctrl+Tab list to distinguish equally named files.Oct 8 2018, 10:07 PM

I like it, definitely an improvement.

Instead of showing "etc", could we still have "/etc"? I think this one more character won't hurt. If we really omit a leading folder, we could have .../some/folder... (as idea)

Instead of showing "etc", could we still have "/etc"? I think this one more character won't hurt. If we really omit a leading folder, we could have .../some/folder... (as idea)

+1 for adding the leading '/' when it represents the root directory

gregormi edited the test plan for this revision. (Show Details)Oct 13 2018, 8:36 AM
gregormi updated this revision to Diff 43523.EditedOct 13 2018, 8:41 AM
gregormi edited the summary of this revision. (Show Details)

Good idea.

  • keep / at the beginning if only this would have been stripped
cullmann accepted this revision.Oct 13 2018, 2:11 PM
cullmann added a subscriber: cullmann.

Looks good to me, much nicer than before.
Thanks!

This revision is now accepted and ready to land.Oct 13 2018, 2:11 PM

Yes, please push. In a separate patch, could you possibly look at KateTabSwitcherTest::testLongestCommonPrefix() again? It looks as if you could use QFETCH along eith a function KateTabSwitcherTest::testLongestCommonPrefix_data()? QFETCH is typically used when testing a data set.

gregormi updated this revision to Diff 43598.Oct 14 2018, 5:06 PM
  • clean code and add comments
gregormi retitled this revision from POC: Show partial path in Tabswitcher Ctrl+Tab list to distinguish equally named files to Show partial path in Tabswitcher Ctrl+Tab list to distinguish equally named files.Oct 14 2018, 5:09 PM
gregormi edited the summary of this revision. (Show Details)
gregormi edited the summary of this revision. (Show Details)Oct 14 2018, 5:13 PM
gregormi updated this revision to Diff 43602.Oct 14 2018, 5:16 PM
gregormi edited the summary of this revision. (Show Details)
  • revert unrelated change
This revision was automatically updated to reflect the committed changes.

Yes, please push. In a separate patch, could you possibly look at KateTabSwitcherTest::testLongestCommonPrefix() again? It looks as if you could use QFETCH along eith a function KateTabSwitcherTest::testLongestCommonPrefix_data()? QFETCH is typically used when testing a data set.

Cool stuff. Created the patch here: https://phabricator.kde.org/D16209

Hi Gregor, I just did a post-review again, and noticed several issues I think we should address. I have a patch ready for most of the stuff, but did not yet post it on Phabricator, since I indeed use KTextEditor::Document as member pointer in the FileListItem, and its constructor now reads the icon etc from the document, which destroys the tsttestapp demo. So the comments are just here to make you aware of the issues. We can have the real discussion when I post the patch.

addons/tabswitcher/tabswitcher.cpp
144–147

Do you see the memory leak? item is never deleted, since you create a copy in ::insertRow().

addons/tabswitcher/tabswitcher.h
142

Since we also have a TabSwitcherTreeView which is not in the detail namespace, I think putting the model into the detail namespace adds more inconsistency than it solves. In fact, Given the tabswitcher plugin is just 3 files, it is arguable what is a "detail" and what not. In other words: Overengineering? :-)

addons/tabswitcher/tabswitcherfilesmodel.cpp
34

What license is this? Since we seem to reuse logs of code, this is important :-)

addons/tabswitcher/tabswitcherfilesmodel.h
34

Indeed using a QStandardItem when NOT using a QStandardItemModel is more confusing than it helps, since some data is fetched directly from the item's member variables, and some data via the data() accessor. I strongly suggest to not do this: Since we derive from QAbstractTableModel now, I'd argue that using QStandardItem is wrong in terms of "that's not how it was designed".

Mixing ->data() and direct member access is rather confusing.

Hi Gregor, I just did a post-review again, and noticed several issues I think we should address. I have a patch ready for most of the stuff, but did not yet post it on Phabricator, since I indeed use KTextEditor::Document as member pointer in the FileListItem, and its constructor now reads the icon etc from the document, which destroys the tsttestapp demo. So the comments are just here to make you aware of the issues. We can have the real discussion when I post the patch.

Hi Dominik, thanks! I was indeed a bit surprised that my patch in this area of Kate received only so few remarks before being accepted. :-)

gregormi added inline comments.Oct 17 2018, 9:47 PM
addons/tabswitcher/tabswitcherfilesmodel.cpp
34

On https://helloacm.com/c-coding-exercise-longest-common-prefix/ the footer says "All rights Reserved © 2014~ 2018"
On https://helloacm.com/coding-exercise-source-codes-and-binaries/ there is a list of all examples and it says:

"The programming examples are as ‘it is’. I do not hold any responsibility for damages/loss caused by running them."

Does the 'as it is' already means we can use it? What if I reimplement it and still come up with a similar solution? Should I try to contact the site owner?