Outline: Sorted function list
ClosedPublic

Authored by kfunk on Dec 14 2016, 12:54 PM.

Details

Summary

Addressed the bug: 319404 (https://bugs.kde.org/show_bug.cgi?id=319404)

-> As suggested by Kevin, wrapped the existing QuickOpenModel with the QSortFilterProxyModel to produce a sorted list of functions in the Outline drop down menu.
-> Converted the indexes from the Proxy Model to the Source Model and vice-versa where ever necessary.
-> Analyzed the use cases of the Outline menu, and modified all it's functionalities and features corresponding to the Proxy Model.

Test Plan

-> Tested the working of the Outline and QuickOpen menu on a large project base.
-> Tested for the smooth functioning of widget expansion, both complete and partial.
-> Tested for Double Click, Single Click, Key Press, and other events.

Diff Detail

Repository
R33 KDevPlatform
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
nellex updated this revision to Diff 8998.Dec 14 2016, 12:54 PM
nellex retitled this revision from to Outline: Sorted function list.
nellex updated this object.
nellex edited the test plan for this revision. (Show Details)
nellex added reviewers: KDevelop, kfunk.
nellex added a project: KDevelop.
nellex added a subscriber: KDevelop.
kfunk set the repository for this revision to R33 KDevPlatform.Dec 14 2016, 1:39 PM
brauch added a subscriber: brauch.Dec 16 2016, 1:44 PM

Will have a closer look later, but wasn't the idea not to sort the list on purpose, and have the same order as in the file? I think that does have a certain value to it ...
Not that much opposed to changing it either, though. Milian might also have a relevant opinion.

kfunk edited edge metadata.Dec 16 2016, 1:47 PM
In D3673#69263, @brauch wrote:

Will have a closer look later, but wasn't the idea not to sort the list on purpose, and have the same order as in the file? I think that does have a certain value to it ...

I agree that both versions are useful. Personally I'd rather see a sorted list here, though.

Make it configurable, and use a QIdentityProxyModel if sorting is disabled?

Not that much opposed to changing it either, though. Milian might also have a relevant opinion.

brauch accepted this revision.Dec 29 2016, 3:24 PM
brauch added a reviewer: brauch.

Looks good to me except what is noted below.

plugins/quickopen/expandingtree/expandingwidgetmodel.cpp
92

rename this to "sourceIndex" or so, having "index" and "idx" in the scope is confusing.

176

use a local variable

This revision is now accepted and ready to land.Dec 29 2016, 3:24 PM
nellex updated this revision to Diff 9469.Dec 29 2016, 6:43 PM
nellex edited edge metadata.

Minor fixes as suggested by brauch.

Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptDec 29 2016, 6:43 PM
kfunk added a comment.Dec 30 2016, 8:51 AM

Just tried to test -- doesn't apply cleanly on kdevplatform master -- Could you rebase the patch?

nellex updated this revision to Diff 9688.Jan 4 2017, 7:08 AM
nellex marked 2 inline comments as done.

Rebase patch.

IMO with 5.1 branched off, you can now just push this to master, we will notice breakage fast enough.

nellex updated this revision to Diff 9728.Jan 4 2017, 7:46 PM

Rebase branch with master

kfunk commandeered this revision.Jan 5 2017, 12:48 AM
kfunk edited reviewers, added: nellex; removed: kfunk.

Taking over, I have an alternative patch. Took me a while, too...

kfunk updated this revision to Diff 9734.Jan 5 2017, 12:54 AM

Changes:

  • Make sure we pass the correct QModelIndexes to the model
  • Make it possible to enable/disable sorting from the outside
  • Only enable sorting for the outline
    • Use QSFPM for Outline, QIdentityProxyModel for the rest

Unfortunately a lot of awful code, but that's what you get when the underlying model is already not well separated from the view...

Tested both outline + quick open extensively, seems to work

kfunk added a comment.Jan 5 2017, 12:54 AM

@brauch Please give this another review

brauch added a comment.Jan 6 2017, 1:37 AM

Hmm, quite a diff :D
I have read through it, what I've seen looks sensible except the two places I commented. I think it's ok to put it into master now, and see if it breaks...

plugins/quickopen/expandingtree/expandingtree.cpp
45

This makes no sense, first you deference it and then you have a check below

plugins/quickopen/quickopenmodel.cpp
425

huh :D

kfunk marked 2 inline comments as done.Jan 6 2017, 8:26 AM

Will push

plugins/quickopen/quickopenmodel.cpp
425

Well, in a release build index will be unused (due to #define Q_ASSERT) -- that's why you'll need Q_UNUSED

This revision was automatically updated to reflect the committed changes.
kfunk marked an inline comment as done.
brauch added a comment.Jan 6 2017, 9:16 AM

I was more wondering about the function always returning true ;) But I guess it makes some kind of sense semantically.

zwabel added a subscriber: zwabel.Jan 11 2017, 3:15 PM

Hi, sorry that I'm late on this, but this is wrong.

The outline is ment to be a more compact view of the file, sorted in the same way. That means, if you click into the outline, the current function is selected in it, and you get an overview of the surrounding. For example, you can press CTRL+ALT+N, up-arrow, enter, to jump to the function above. Sorting completely breaks this.

Such a sorted list does not belong into the outline, but rather into the quick open widget at the left side of it. It would naturally fit there, being just like "CTRL+Alt+M", just with a new "Scope" called "Current File". Btw. I noticed that the selection of scopes in Quickopen was broken at some point. If it wouldn't be broken, a very similar functionality would already be available in Quickopen, by selecting "Currently Open" for "Scope", and "Functions" for "Items".

kfunk added a comment.Jan 11 2017, 4:19 PM
In D3673#76450, @zwabel wrote:

Hi, sorry that I'm late on this, but this is wrong.

The outline is ment to be a more compact view of the file, sorted in the same way. That means, if you click into the outline, the current function is selected in it, and you get an overview of the surrounding. For example, you can press CTRL+ALT+N, up-arrow, enter, to jump to the function above. Sorting completely breaks this.

Hmm, I never felt the urge to do this actually. *Might* be useful for some people , but definitely not for all. See below.

Such a sorted list does not belong into the outline, but rather into the quick open widget at the left side of it. It would naturally fit there, being just like "CTRL+Alt+M", just with a new "Scope" called "Current File". Btw. I noticed that the selection of scopes in Quickopen was broken at some point. If it wouldn't be broken, a very similar functionality would already be available in Quickopen, by selecting "Currently Open" for "Scope", and "Functions" for "Items".

IMO we shouldn't overload the Quick Open widget with even more functionality, it's already complex enough as-is.

What about just making the sorting optional in the outline and add a push button at the below the list view? There are already combo boxes in Quick Open, so it shouldn't be difficult to add. Note we already have similar functionality in the outline *tool view*, where we can toggle sorting by simply pressing a button.

Hi Kevin,

the point is, that the mere distinction between the outline line and the quickopen line at the left of it is, that the outline is 'local', whereas the quickopen is 'global'. Sorting the outline basically makes it quickopen just with a very restricted scope, so it puts into question whether this additional line-edit in the UI is necessary at all.

At least when the user clicks in the line, I think it should be 'local' and therefore imitate the structure in the code.

Adding a combo-box there, like the Quickopen has, might be an option, but it would complicate the UI and add more clutter, for functionality which is basically already available.

I wonder whether this whole feature request wouldn't be fully satisfied when Quickopen with "Scope=Currently Open" and "Items=Functions+Classes" would be fixed, and got an own shortcut?

Otherwise, it might make sense to simply add another shortcut for "Sorted Outline", but not overload the UI with it.

arrowd added a subscriber: arrowd.Jan 12 2017, 12:17 PM

The outline is ment to be a more compact view of the file, sorted in the same way. That means, if you click into the outline, the current function is selected in it, and you get an overview of the surrounding.

Visual Studio does sort outline list, just a note. And I, personally, find it neater.

AFAIK the visual studio outline is a list for mouse based semi-global navigation, more similar to the 'Classes' toolbar we have in KDevelop at the left side. Which could probably use some love.

The KDevelop outline is for _keyboard_ based _local_ navigation in the document, so it caters different needs. It's a better alternative to the "Previous Function" and "Next Function" shortcuts.

Btw. this checkin also caused another regression:

The outline view normally shows the name/path of the function you're currently in.

When you click into it, this function should also be the the currently selected function, and the view should be scrolled such that it's visible.

mwolff added a subscriber: mwolff.Jan 20 2017, 10:28 PM

FWIW, I'm with zwabel on this matter

rjvbb added a subscriber: rjvbb.Oct 1 2017, 9:47 AM
In D3673#76467, @kfunk wrote:

What about just making the sorting optional in the outline and add a push button at the below the list view?

+++
(emphasis on optional if with sorting you mean the alphanumerical sorting).

zwabel added a comment.Oct 9 2017, 2:19 PM

Hi guys, I propose that we disable the sorting again, because it breaks the local browsing by keyboard usecase, i.e. you're in a function, you press CTRL+ALT+N, the current function is selected and you see the surround functions, you push up/down to jump to adjacent functions. This is what this keyboard based outline is mainly useful for.

For bug: 319404 which was addressed by this, I just discovered that we already have the outline sidebar, which has a button at the top to enable alphanumerical sorting. That seems to exactly address the issue of the requester, so why should his keyboard-controlled outline be sorted as well?

kfunk added a comment.Oct 9 2017, 3:03 PM

Hi guys, I propose that we disable the sorting again, because it breaks the local browsing by keyboard usecase, i.e. you're in a function, you press CTRL+ALT+N, the current function is selected and you see the surround functions, you push up/down to jump to adjacent functions. This is what this keyboard based outline is mainly useful for.

I'd propose to make the sorting optional. I don't agree that this is what the outline is "mainly used for".

For bug: 319404 which was addressed by this, I just discovered that we already have the outline sidebar, which has a button at the top to enable alphanumerical sorting. That seems to exactly address the issue of the requester, so why should his keyboard-controlled outline be sorted as well?

The top-bar outline is easy to activate/use via keyboard, the sidebar outline isn't. I still find it useful to have the top-bar outline sorted as well when I use it via keyboard.