Make CTags/Projects context menu more accessible
AbandonedPublic

Authored by cullmann on Dec 2 2018, 1:43 PM.

Details

Reviewers
sars
gregormi
Group Reviewers
Kate
Summary
  • CTags: Move the actions out of the submenus to the root level of the context menu
  • Projects: Move the Lookup action to root level of context menu

Both: Remove the code that inserts the text that is about to be searched to the action text.
Reasons:

  1. Those actions are super-useful
  2. Improves the discoverability of the Projects Code Index
  3. It simplifies the code by removing the dynamic item text logic (the user will see what is being searched as soon as the search is started anyway).

Screenshot:

Diff Detail

Repository
R40 Kate
Branch
arcpatch-D17308
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5829
Build 5847: arc lint + arc unit
gregormi created this revision.Dec 2 2018, 1:43 PM
Restricted Application added a project: Kate. · View Herald TranscriptDec 2 2018, 1:43 PM
Restricted Application added a subscriber: kwrite-devel. · View Herald Transcript
gregormi requested review of this revision.Dec 2 2018, 1:43 PM
gregormi edited the summary of this revision. (Show Details)Dec 2 2018, 1:45 PM
gregormi added a reviewer: Kate.
gregormi edited the summary of this revision. (Show Details)
ngraham added a subscriber: ngraham.Dec 2 2018, 6:41 PM

Oh wow, this would be awesome. This is only visible if the CTags plugin is active, right?

With this change, it might make sense to remove the CTags: on the beginning of the labels.

addons/kate-ctags/kate_ctags_view.cpp
66

While you're touching this user-facing string, let's fix the grammar for all the places where it's currently wrong, because "lookup" is a noun, not a verb.

Lookup Selected Text -> Look Up Selected Text

sars added a subscriber: sars.Dec 2 2018, 7:36 PM

What if you have Project, CTags and the new Zeal plugins active? You need to differentiate them somehow.

Optimally for Kate, we would get the go to definition/declaration/back to the project plugin and we would not need the CTags plugin any more. If we at some point would have both plugins with multiple actions, the menu could become quite long.

As it is right now for the project plugin, it is a bit strange to have a sub-menu for one action, but we could get more....

dhaumann added inline comments.
addons/kate-ctags/ui.rc
3

I think you have to increase the version number, otherwise caching of the ui files by KXMLGUI will lead to issues.

addons/project/ui.rc
3

Same here: version++

gregormi updated this revision to Diff 47133.Dec 8 2018, 5:18 PM
gregormi edited the summary of this revision. (Show Details)
  • Fix texts and increase versions in .rc files
gregormi edited the summary of this revision. (Show Details)Dec 8 2018, 5:19 PM
gregormi marked 3 inline comments as done.Dec 19 2018, 11:37 PM
In D17308#370361, @sars wrote:

What if you have Project, CTags and the new Zeal plugins active? You need to differentiate them somehow.

See latest screenshot. Would that be ok?

As it is right now for the project plugin, it is a bit strange to have a sub-menu for one action, but we could get more....

Depending on what those more actions do, top-level could still be ok; I would decide if we have concrete actions.

In D17308#370361, @sars wrote:

What if you have Project, CTags and the new Zeal plugins active? You need to differentiate them somehow.
[...]
As it is right now for the project plugin, it is a bit strange to have a sub-menu for one action, but we could get more....

@sars: The names of the menu items are distinct. Is your last statement to be meant as an objection to this change or can I proceed here?

sars accepted this revision.Jan 13 2019, 10:38 AM

I'm not entirely sure removing the feature of having the lookup word in the menu is an improvement, but I just noticed that the feature is broken and only works when a word is selected.

If you think this is an improvement (with that one string fixed), I'm ok with this going in. I still mostly just use the shortcuts :)

addons/kate-ctags/kate_ctags_view.cpp
66

This change is not entirely correct as you do not need to select any text. You can look up the word containing the text cursor. That is why it says "current" and not "selected" text.

I would suggest "Look Up Current Text"

This revision is now accepted and ready to land.Jan 13 2019, 10:38 AM
gregormi added a subscriber: rjvbb.Jan 13 2019, 7:31 PM

Thanks Kåre for the review. I'll update the change.

But first I have to solve merge conflicts with current master because of 9af81dd535c71da75a7caeec08b46a1212f53399 and 3cd03f408eed2d66ae008fb8349f8b5af24260e9.
@rjvbb Could you check if in the current master works as intended? For me it looks like this:


(Project plugin context menu item is missing)

Should I check if current master works as I intended, WITHOUT your patch, because it does not apply to current master.

Personally I see no reason to "unroll" the CTags submenu, nor to remove the dynamic menu items, and I see reason not to change these aspects.
So -2 (2 * -1) from me for the original idea.

I like to see what the search is going to be for before starting it. That search might open (or change) the toolview and it will probably open a document. Both are effects that are more disruptive to correct than closing the submenu.

Remember that this plugin can also be used in KDevelop where its menu will appear in menubar and context menus that are considerably more cluttered already than the one in the screenshot you're showing me. In addition, the CTags plugin has a bit of an "if all else fails" nature in KDevelop, which already has much better integrated similar functionality for a few supported languages. IOW, the CTags functionality should really be in a submenu there.

In short, to come back to my -2: change what you want but try to keep things as unchanged as possible when the plugin is loaded in KDevelop. This will also allow the plugin to integrate properly with Kate's project manager plugin (which will probably never be supported in KDevelop).
The host application can be detected in several ways, the most reliable is in the ctor of the KTextEditor::Plugin child class. From my zealsearch plugin:

// parent->metaObject()->className() == "KatePluginManager" in kate
// parent->metaObject()->className() == "KDevelop::Core" in KDevelop
cullmann requested changes to this revision.Apr 12 2019, 8:06 PM
cullmann added a subscriber: cullmann.

I reconsider, I see no real value in removing the layout beside having a more densely populated toplevel menu.
Could we close this?

This revision now requires changes to proceed.Apr 12 2019, 8:06 PM

There are two things left that could be useful:

  1. "simplifies the code by removing the dynamic item text logic"
  2. The text change from "Lookup" to "Look Up"

Since I have no time to bring this to an timely end, the ticket can be closed.

rjvbb added a comment.Apr 14 2019, 5:05 PM
  1. "simplifies the code by removing the dynamic item text logic"

IMHO that would be one of places where the code can be simplified by removing features that would be missed.

cullmann commandeered this revision.Wed, May 8, 5:53 PM
cullmann edited reviewers, added: gregormi; removed: cullmann.

Lets close this ATM.
If one has time to improve here again, one can reopen it.

cullmann abandoned this revision.Wed, May 8, 5:53 PM