Make the text of "View Settings" action more specific
AbandonedPublic

Authored by trmdi on Mon, Feb 11, 4:55 AM.

Details

Reviewers
mvourlakos
Summary

If it's a dock -> Dock Settings...
If it's a panel -> Panel Settings...

Diff Detail

Repository
R878 Latte Dock
Lint
Lint Skipped
Unit
Unit Tests Skipped
trmdi created this revision.Mon, Feb 11, 4:55 AM
Restricted Application added a project: Plasma. · View Herald TranscriptMon, Feb 11, 4:55 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
trmdi requested review of this revision.Mon, Feb 11, 4:55 AM
trmdi updated this revision to Diff 51389.Mon, Feb 11, 5:44 AM
trmdi added inline comments.Mon, Feb 11, 8:12 AM
app/view/view.cpp
333 ↗(On Diff #51389)

I've found a bug here: when inEditMode, m_behaveAsPlasmaPanel is always "false".
Could you explain why?

trmdi planned changes to this revision.Mon, Feb 11, 12:26 PM

Personally I dont like the approach, this wont work in some cases e.g. when right clicking plasma taskmanager.
This actions are located at : https://phabricator.kde.org/source/latte-dock/browse/master/containmentactions/contextmenu/menu.cpp

I would try first in there to check if it is possible to track a parent e.g. of Latte::View

app/view/view.cpp
333 ↗(On Diff #51389)

behaveAsPlasmaPanel as a variable defines only the behavior of the view and not the type. Currently the Latte::View does not have a type in order to be used by other elements. How this could be done is the following:

  1. add in Latte::View a type variable which is going to be available through qml to be altered
  2. containment qml is going to be responsible for [1] to be set propertly
  3. [1] wont be altered in edit mode like behaveAsPlasmaPanel is

I can do [1-3] if you want in order to build after.

In edit mode the behaveIsPlasmaPanel is false because it editMode the view must not function as a plasma panel in order to support the animations correctly, set its sizes properly, set its masks correctly etc.

ok, just checked the previous mentioned class has access to containment() that means that through its configuration the menu could be informed for the type, so directly access to Latte::View is not needed...

trmdi added a comment.EditedMon, Feb 11, 5:00 PM

Personally I dont like the approach, this wont work in some cases e.g. when right clicking plasma taskmanager.

but when right clicking on plasma taskmanager, there is not any Latte's action. This patch only changes the text of the action when it's available.
Is it another bug?

trmdi added a comment.Mon, Feb 11, 5:05 PM

ok, just checked the previous mentioned class has access to containment() that means that through its configuration the menu could be informed for the type, so directly access to Latte::View is not needed...

wait to see your approach.

trmdi marked 2 inline comments as done.Mon, Feb 11, 5:09 PM
trmdi updated this revision to Diff 51431.Mon, Feb 11, 6:14 PM
mvourlakos requested changes to this revision.Mon, Feb 11, 6:16 PM

This patch should not touch Latte::View at all. All the code must be in the containmentactions/contextmenu.cpp class

This revision now requires changes to proceed.Mon, Feb 11, 6:16 PM
trmdi updated this revision to Diff 51456.Tue, Feb 12, 5:22 AM
trmdi updated this revision to Diff 51458.Tue, Feb 12, 6:01 AM
trmdi updated this revision to Diff 51459.Tue, Feb 12, 6:20 AM
trmdi updated this revision to Diff 51464.Tue, Feb 12, 6:48 AM
trmdi updated this revision to Diff 51481.Tue, Feb 12, 10:41 AM

for Latte panels after startup or after switching layouts if the user right clicks the panel the menu is appearing correctly as:

Panel Setings... ?

containmentactions/contextmenu/menu.cpp
92

why this is needed?

93

style issue. even for single lines ifs

if (..) {

}
trmdi added a comment.EditedTue, Feb 12, 1:08 PM

for Latte panels after startup or after switching layouts if the user right clicks the panel the menu is appearing correctly as:

Panel Setings... ?

Whenever there is a valid viewType in the layout file.
There is a case that viewType doesn't exist, for this case, you have to reload the view to have it created. (or close the Setting window)
Do you have a better way to detect when viewType changes?

containmentactions/contextmenu/menu.cpp
92
  • when the user opens View Settings, there is a case that he change the viewType, so we should set the text again.
trmdi updated this revision to Diff 51498.Tue, Feb 12, 1:11 PM

Do you have a better way to detect when viewType changes?

I don't think it is needed, you can try to update the action text just a little before the menu is shown

Qmenu has an aboutToShow signal and containment has as a contextuakActionsAboutToShow signal

I would try these two first

trmdi added a comment.EditedTue, Feb 12, 2:12 PM

I don't think it is needed, you can try to update the action text just a little before the menu is shown

Qmenu has an aboutToShow signal and containment has as a contextuakActionsAboutToShow signal

You meant to run setConfigureActionText() whenever the menu/action is about to show?
It will have to read the config whenever you right click. If there isn't any issue with reading the config again and again, e.g. disk reading... then we could set the text inside the Menu::contextualActions()

trmdi updated this revision to Diff 51515.Tue, Feb 12, 3:06 PM
mvourlakos added inline comments.Tue, Feb 12, 3:55 PM
containmentactions/contextmenu/menu.cpp
91

is this still needed if we check the value each time we show up the context menu?

trmdi added inline comments.Tue, Feb 12, 4:03 PM
containmentactions/contextmenu/menu.cpp
91

Yes, it's needed. Because when you're opening the Setting window and you right click on the view, it will close the Setting window, not a nice behavior. So we should hide it when the Setting window is opening.

mvourlakos added inline comments.Tue, Feb 12, 4:36 PM
containmentactions/contextmenu/menu.cpp
68

when we add character & in the menus it means that a modifier+&Letter activates that option what is the shortcut that add widgets?

78

when we add character & in the menus it means that a modifier+&Letter activates that option what is the shortcut that shows dock settings? it has been chosen to be Meta+A so that change does not respond to something

84

when we add character & in the menus it means that a modifier+&Letter activates that option what is the shortcut that shows Latte layouts menu? none, this shouldnt be applied also

91

ok this a behavior change irrelevant with the patch but ok we can keep it

mvourlakos added inline comments.Tue, Feb 12, 4:38 PM
containmentactions/contextmenu/menu.cpp
132

same thing for these & characters, do not respond to any shortcut so they should be removed

mvourlakos added inline comments.Tue, Feb 12, 4:47 PM
containmentactions/contextmenu/menu.cpp
92

in this slot is also needed:

m_configureAction->setEnabled(true);

there are cases that the Dock Settings is shown when !configuring but it is not enabled

trmdi added inline comments.Tue, Feb 12, 4:58 PM
containmentactions/contextmenu/menu.cpp
68

For example, "&Add widgets..."

  • shortcut: Alt+a
  • the shortcut only works when that menu is showing
  • it does not change anything else
trmdi added inline comments.Tue, Feb 12, 5:08 PM
containmentactions/contextmenu/menu.cpp
92

I don't understand. my code only change the visible property, why does it have to handle the "enable" one?

You meant the setting is showing but configuring==false? Sound weird. How to make it happen?

@trmdi please do me a favour:

  1. close this review and open a clean new one because for some reason when trying to test it in my system includes also one old commit that I added the ViewType option
  2. no problem with & characters you can add them
  3. no problem to hide the Dock/Panel View when (configuring)
  4. please dont use the config file to access the viewType, I found an alternative way that it is in the master, we sniff now the viewType through dbus contextMenuData. The menu sends the containment->id() and when contextMenuData are requested these data include also the view type.
  5. For the setVisible(true) and how this bug occurs we will discuss it again at the new review
trmdi abandoned this revision.Wed, Feb 13, 1:44 AM