Differential D26238

Improve context menu
ClosedPublic
Actions

Authored by trmdi on Dec 27 2019, 4:07 AM.

Details

Reviewers
mvourlakos
Group Reviewers
Latte Dock
VDG
Commits
R878:fe5dbf9f2b96: Improve context menu
Summary
  • Add Configure Latte Global Settings... because it's too hidden in Layout > Configure and does not relate to the Layout menu.
  • Insert a new Separator to separate between Global settings and View settings (see screenshot)
  • Rename Panel/Dock Settings to Edit Panel/Dock (Verb + Noun)
  • Rename Layout > Configure... to Layout > Manage Layouts (Verb + Noun)

Menu Items Order:

a.  -separator- 
b. Layouts->
c. Configure Application...
e.  -separator-
f. Add Widgets...
g. Edit Dock...
h.  -separator- [only for Tasks plasmoid]
i.  Close... [only for Tasks plasmoid]

BUG: 415584

Test Plan

Before:

After:
a. context menu of applets


b. context menu of a task that hasn't/has one window shown


c. context menu of Latte Tasks plasmoid when used on the desktop

Diff Detail

Repository
R878 Latte Dock
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
trmdi created this revision.Dec 27 2019, 4:07 AM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 27 2019, 4:07 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
trmdi requested review of this revision.Dec 27 2019, 4:07 AM
trmdi edited the summary of this revision. (Show Details)Dec 27 2019, 4:08 AM
trmdi edited the test plan for this revision. (Show Details)Dec 27 2019, 5:00 AM
trmdi edited the summary of this revision. (Show Details)Dec 27 2019, 5:04 AM
ndavis added a subscriber: ndavis.Dec 27 2019, 5:06 AM

This seems inconsistent with the Plasma panel context menu:

trmdi edited the summary of this revision. (Show Details)Dec 27 2019, 5:07 AM
ndavis added a comment.Dec 27 2019, 5:08 AM

In the plasma panel context menu, add widgets and edit panel are below the separator

trmdi added a comment.EditedDec 27 2019, 5:13 AM
In D26238#583389, @ndavis wrote:

In the plasma panel context menu, add widgets and edit panel are below the separator

Maybe I need to create a new separator at the line 126 instead of reusing it. Is it ok if there are 2 separators in a context menu?
What about the other changes?

ndavis added a comment.EditedDec 27 2019, 5:17 AM
In D26238#583390, @trmdi wrote:
In D26238#583389, @ndavis wrote:

In the plasma panel context menu, add widgets and edit panel are below the separator

Maybe I need to create a new separator at the line 126 instead of reusing it. Is it ok if there are 2 separators in a context menu?

Sounds reasonable, although I'm not sure they're unrelated enough that they need an extra separator. I'll leave it up to your judgement.

What about the other changes?

They look fine.

mvourlakos requested changes to this revision.Dec 27 2019, 6:10 AM
mvourlakos added a reviewer: mvourlakos.
This revision now requires changes to proceed.Dec 27 2019, 6:10 AM
trmdi edited the test plan for this revision. (Show Details)Dec 27 2019, 7:09 AM
trmdi updated this revision to Diff 72228.EditedDec 27 2019, 7:09 AM
  • Add a new separator and change addWidget's icon
  • Rename Latte Global Settings to Configure Latte Global Settings (Verb+Noun)
trmdi edited the summary of this revision. (Show Details)Dec 27 2019, 7:10 AM
mvourlakos added a comment.EditedDec 27 2019, 7:13 AM

This commit needs to be split in different commits.
Based on the approach I wil describe the issues of the commit and in what changes I can approve or not.

SPLITTED COMMITS

  1. The showLatteSettingsDialog() is used from more places and you have changed its behavior. The page that

this function takes is used as the first page to show when the window is created, after that it toggles between
tabs. This is used ALSO from [Meta+W] global shortcut and Latte trademark icon in Dock Settings in order to show
the Latte Global Preferences. From my understanding you need to create a new function that is not a toggler but
rather sets specific settings tab all the time

  1. The context menu changes MUST be the same in TWO different places, because otherwise showing context menu will be different between tasks and applets.
a. containmentactions/contextmenu/menu.cpp
b. plasmoid/package/contents/ui/ContextMenu.qml
  1. Renames
a. Layouts->Configure TO Layouts->Manage Layouts...
b. Dock Settings -> Edit Dock... 
c. Latte Global Settings... -> Configure Application....
  1. Order:
a.  -separator-
b. Layouts->
c. Configure Application...
d. Add Widgets...
e. Edit Dock...
f.  -separator- [only for Tasks plasmoid]
g.  Close... [only for Tasks plasmoid]
mvourlakos added a comment.Dec 27 2019, 7:16 AM

the [Verb+Noun] sounds logical, updated names in my last comment

trmdi added a comment.Dec 27 2019, 7:25 AM
  1. It still works for other cases.
  1. "Configure Application" sounds a bit unclearly. And it can confuse users with "Configure Application Dashboard".

"Configure Latte Global Settings..." is too long?

trmdi added a comment.EditedDec 27 2019, 7:32 AM
  1. I want to separate between View's items (Add widget, Edit View) and Latte's items (Layouts, Configure Latte) because they are two different groups (one is the global setting, one is the view setting)

(See the After screenshot)

mvourlakos added a comment.Dec 27 2019, 7:52 AM
In D26238#583413, @trmdi wrote:
  1. It still works for other cases.

Do you cycle in settings tabs with Meta+W, between Layouts and Preferences ?

  1. "Configure Application" sounds a bit unclearly. And it can confuse users with "Configure Application Dashboard". "Configure Latte Global Settings..." is too long?

it is too long... I can accept suggestions for [Verb+Noun] cases

In D26238#583414, @trmdi wrote:
  1. I want to separate between View's items (Add widget, Edit View) and Latte's items (Layouts, Configure Latte) because they are two different groups (one is the global setting, one is the view setting) (See the After screenshot)

I wont object for now, but I will conclude if I will keep the second separator after using the program with the changes for some days. The order of the actions must be the one referenced though.

mvourlakos edited the summary of this revision. (Show Details)Dec 27 2019, 7:54 AM
mvourlakos added inline comments.Dec 27 2019, 8:04 AM
app/layouts/manager.cpp
419

We could try to rename the function to:

void Manager::showLatteSettingsDialog(int firstPage, bool cycleBetweenPages = false)

when cycleBetweenPages = true then the current behavior is applied, in any other case the specified firstPage is shown

app/settings/settingsdialog.cpp
365

leave this function untouched and introduce a new function called,

setCurrentPage(int page)
app/settings/settingsdialog.h
58–59

leave this function untouched and introduce a new function called,

setCurrentPage(int page)

trmdi updated this revision to Diff 72238.Dec 27 2019, 9:51 AM
  • Address comments
trmdi marked 3 inline comments as done.Dec 27 2019, 9:56 AM
trmdi edited the test plan for this revision. (Show Details)Dec 27 2019, 9:58 AM
trmdi edited the test plan for this revision. (Show Details)
trmdi edited the summary of this revision. (Show Details)Dec 27 2019, 10:06 AM
mvourlakos added a comment.EditedDec 27 2019, 10:39 AM

@trmdi based on your last commit:

  1. you have made all showLatteSettingsDialog callers as togglers, meaning that : IF Latte Layouts manager is already shown and the user tries Layouts->Manage Layouts then the settings tab will switch to Latte Preferences window, is that the behavior you want?

If I understood correctly then I think we want the following:

a. menu -> Layouts->Manage Layouts = ALWAYS shows the Layouts tab
b. menu -> Configure Latte... = ALWAYS shows there Preferences tab
c. clicking the Latte trademark in Dock config window = ALWAYS shows there Preferences tab
d. using [Meta+W]  = Toggles between Layouts and Preferences tab
  1. Please provide also three screenshots from this commit changes:
a. context menu of applets
b. context menu of a task that has one window shown
c. context menu of Latte Tasks plasmoid when used on the desktop
trmdi updated this revision to Diff 72244.Dec 27 2019, 1:13 PM
  • Call toggleCurrentPage() only when using the shortcut
mvourlakos added inline comments.Dec 27 2019, 1:19 PM
app/layouts/manager.h
114

please fix type "toggleCurrentPage"

trmdi marked an inline comment as done.Dec 27 2019, 1:26 PM
mvourlakos added a comment.Dec 27 2019, 1:29 PM

look ok! Can you please provide also the three screenshots mentioned earlier for reference?

trmdi edited the test plan for this revision. (Show Details)Dec 27 2019, 1:30 PM
In D26238#583520, @mvourlakos wrote:

look ok! Can you please provide also the three screenshots mentioned earlier for reference?

Done.

mvourlakos accepted this revision.Dec 27 2019, 1:40 PM

looks ok...

I am not that fond of the second line separator but we can discuss this again in the future.

This revision is now accepted and ready to land.Dec 27 2019, 1:40 PM
trmdi added a comment.Dec 27 2019, 2:05 PM
In D26238#583386, @ndavis wrote:

This seems inconsistent with the Plasma panel context menu:

Do you know what icon is used for the "Edit Panel" item in this screenshot?

mvourlakos added a subscriber: ngraham.Dec 27 2019, 2:21 PM
In D26238#583529, @trmdi wrote:
In D26238#583386, @ndavis wrote:

This seems inconsistent with the Plasma panel context menu:

Do you know what icon is used for the "Edit Panel" item in this screenshot?

@ngraham do you know which icon this is it?

mvourlakos added a comment.Dec 27 2019, 2:45 PM

@trmdi I used Cuttlefish app, it points to "edit"

trmdi added a comment.Dec 27 2019, 3:27 PM
In D26238#583551, @mvourlakos wrote:

@trmdi I used Cuttlefish app, it points to "edit"

Maybe it's document-edit.
(See this: https://api.kde.org/frameworks/plasma-framework/html/containment_8cpp_source.html#l00127)

mvourlakos added a comment.Dec 27 2019, 3:31 PM
In D26238#583555, @trmdi wrote:
In D26238#583551, @mvourlakos wrote:

@trmdi I used Cuttlefish app, it points to "edit"

Maybe it's document-edit.
(See this: https://api.kde.org/frameworks/plasma-framework/html/containment_8cpp_source.html#l00127)

Correct

trmdi updated this revision to Diff 72253.Dec 27 2019, 3:33 PM
  • Change icon of the configure action
trmdi added a comment.Dec 27 2019, 3:37 PM
In D26238#583556, @mvourlakos wrote:
In D26238#583555, @trmdi wrote:
In D26238#583551, @mvourlakos wrote:

@trmdi I used Cuttlefish app, it points to "edit"

Maybe it's document-edit.
(See this: https://api.kde.org/frameworks/plasma-framework/html/containment_8cpp_source.html#l00127)

Correct

Could I land it now?

mvourlakos added a comment.Dec 27 2019, 3:38 PM
In D26238#583563, @trmdi wrote:
In D26238#583556, @mvourlakos wrote:
In D26238#583555, @trmdi wrote:
In D26238#583551, @mvourlakos wrote:

@trmdi I used Cuttlefish app, it points to "edit"

Maybe it's document-edit.
(See this: https://api.kde.org/frameworks/plasma-framework/html/containment_8cpp_source.html#l00127)

Correct

Could I land it now?

Of course

Closed by commit R878:fe5dbf9f2b96: Improve context menu (authored by trmdi). · Explain WhyDec 27 2019, 4:41 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 72255

View Options

app/layouts/manager.h

Loading...
View Options

app/layouts/manager.cpp

Loading...
View Options

app/settings/settingsdialog.h

Loading...
View Options

app/settings/settingsdialog.cpp

Loading...
View Options

app/shortcuts/globalshortcuts.cpp

Loading...
View Options

containmentactions/contextmenu/menu.h

Loading...
View Options

containmentactions/contextmenu/menu.cpp

Loading...
View Options

plasmoid/package/contents/ui/ContextMenu.qml

Loading...