D18918: Make the text of "View Settings" action more specific

[Michail Vourlakos]

mvourlakos added a comment.


  @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

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi added inline comments.

INLINE COMMENTS

> mvourlakos wrote in 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

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?

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi added inline comments.

INLINE COMMENTS

> mvourlakos wrote in menu.cpp:68
> when we add character & in the menus it means that a modifier+ 
> activates that option what is the shortcut that add widgets?

For example, " widgets..."

- shortcut: `Alt+a`
- the shortcut only works when that menu is showing
- it does not change anything else

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Michail Vourlakos]

mvourlakos added inline comments.

INLINE COMMENTS

> menu.cpp:92
> +connect(this->containment(), 
> ::Containment::userConfiguringChanged, this, [&](bool configuring){
> +m_configureAction->setVisible(!configuring);
> +});

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

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Michail Vourlakos]

mvourlakos added inline comments.

INLINE COMMENTS

> menu.cpp:132
> +const int viewType = this->containment()->config().readEntry("viewType", 
> static_cast(Latte::Types::DockView));
> +m_configureAction->setText(i18nc("view settings window", viewType == 
> Latte::Types::DockView ? "Dock " : "Panel "));
> +

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

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Michail Vourlakos]

mvourlakos added inline comments.

INLINE COMMENTS

> menu.cpp:68
>  
> -m_addWidgetsAction = new QAction(QIcon::fromTheme("add"), i18n("Add 
> Widgets..."), this);
> +m_addWidgetsAction = new QAction(QIcon::fromTheme("add"), i18n(" 
> Widgets..."), this);
>  m_addWidgetsAction->setStatusTip(i18n("Show Plasma Widget Explorer"));

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

> menu.cpp:78
>  
> -m_configureAction = new QAction(QIcon::fromTheme("configure"), 
> i18nc("view settings window", "View Settings..."), this);
> -m_configureAction->setShortcut(QKeySequence());
> +m_configureAction = new QAction(QIcon::fromTheme("configure"), 
> i18nc("view settings window", "View "), this);
> +

when we add character & in the menus it means that a modifier+ 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

> menu.cpp:84
>  m_layoutsAction = m_switchLayoutsMenu->menuAction();
> -m_layoutsAction->setText(i18n("Layouts"));
> +m_layoutsAction->setText(i18n(""));
>  m_layoutsAction->setIcon(QIcon::fromTheme("user-identity"));

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

> trmdi wrote in 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.

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

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi added inline comments.

INLINE COMMENTS

> mvourlakos wrote in menu.cpp:91
> is this still needed if we check the value each time we show up the context 
> menu?

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.

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Michail Vourlakos]

mvourlakos added inline comments.

INLINE COMMENTS

> menu.cpp:91
> +
> +connect(this->containment(), 
> ::Containment::userConfiguringChanged, this, [&](bool configuring){
> +m_configureAction->setVisible(!configuring);

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

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi updated this revision to Diff 51515.

REPOSITORY
  R878 Latte Dock

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18918?vs=51498=51515

REVISION DETAIL
  https://phabricator.kde.org/D18918

AFFECTED FILES
  containmentactions/contextmenu/menu.cpp

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi added a comment.


  In D18918#410614 , @mvourlakos 
wrote:
  
  > 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 again and again. 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()`

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Michail Vourlakos]

mvourlakos added a comment.


  In D18918#410587 , @trmdi wrote:
  
  > In D18918#410552 , @mvourlakos 
wrote:
  >
  > >
  >
  
  
  
  
  > 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

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi updated this revision to Diff 51498.

REPOSITORY
  R878 Latte Dock

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18918?vs=51481=51498

REVISION DETAIL
  https://phabricator.kde.org/D18918

AFFECTED FILES
  containmentactions/contextmenu/menu.cpp
  containmentactions/contextmenu/menu.h

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi added a comment.


  In D18918#410552 , @mvourlakos 
wrote:
  
  > 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.

INLINE COMMENTS

> mvourlakos wrote in menu.cpp:92
> why this is needed?

- when the user opens View Settings, there is a case that he change the 
viewType, so we should set the text again.

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Michail Vourlakos]

mvourlakos added a comment.


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

INLINE COMMENTS

> menu.cpp:92
> +connect(this->containment(), 
> ::Containment::userConfiguringChanged, this, [&](bool configuring){
> +m_configureAction->setVisible(!configuring);
> +if (!configuring) setConfigureActionText();

why this is needed?

> menu.cpp:93
> +m_configureAction->setVisible(!configuring);
> +if (!configuring) setConfigureActionText();
> +});

style issue. even for single lines ifs

  if (..) {
  
  }

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi updated this revision to Diff 51481.

REPOSITORY
  R878 Latte Dock

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18918?vs=51464=51481

REVISION DETAIL
  https://phabricator.kde.org/D18918

AFFECTED FILES
  containmentactions/contextmenu/menu.cpp
  containmentactions/contextmenu/menu.h

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi updated this revision to Diff 51464.

REPOSITORY
  R878 Latte Dock

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18918?vs=51459=51464

REVISION DETAIL
  https://phabricator.kde.org/D18918

AFFECTED FILES
  containmentactions/contextmenu/menu.cpp
  containmentactions/contextmenu/menu.h

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi updated this revision to Diff 51459.

REPOSITORY
  R878 Latte Dock

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18918?vs=51458=51459

REVISION DETAIL
  https://phabricator.kde.org/D18918

AFFECTED FILES
  containmentactions/contextmenu/menu.cpp
  containmentactions/contextmenu/menu.h

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi updated this revision to Diff 51458.

REPOSITORY
  R878 Latte Dock

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18918?vs=51456=51458

REVISION DETAIL
  https://phabricator.kde.org/D18918

AFFECTED FILES
  containmentactions/contextmenu/menu.cpp
  containmentactions/contextmenu/menu.h

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi updated this revision to Diff 51456.

REPOSITORY
  R878 Latte Dock

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18918?vs=51431=51456

REVISION DETAIL
  https://phabricator.kde.org/D18918

AFFECTED FILES
  containmentactions/contextmenu/menu.cpp
  containmentactions/contextmenu/menu.h

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Michail Vourlakos]

mvourlakos requested changes to this revision.
mvourlakos added a comment.
This revision now requires changes to proceed.


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

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi updated this revision to Diff 51431.

REPOSITORY
  R878 Latte Dock

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18918?vs=51389=51431

REVISION DETAIL
  https://phabricator.kde.org/D18918

AFFECTED FILES
  app/view/contextmenu.cpp
  app/view/view.cpp
  app/view/view.h

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi added a comment.


  In D18918#409852 , @mvourlakos 
wrote:
  
  > 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.

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi added a comment.


  In D18918#409850 , @mvourlakos 
wrote:
  
  > 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 action. This 
patch only changes the text of the action when it's available.
  Is it another bug?

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Michail Vourlakos]

mvourlakos added a comment.


  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...

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Michail Vourlakos]

mvourlakos added a comment.


  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

INLINE COMMENTS

> trmdi wrote in view.cpp:333
> I've found a bug here: when inEditMode, `m_behaveAsPlasmaPanel` is always 
> "false".
> Could you explain why?

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.

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi added inline comments.

INLINE COMMENTS

> view.cpp:333
> +};
> +const QString viewSettingsText(m_behaveAsPlasmaPanel? 
> viewSettingsTexts[1] : viewSettingsTexts[2]);
> +const QString strippedText(a->text().replace("&", ""));

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

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi updated this revision to Diff 51389.

REPOSITORY
  R878 Latte Dock

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18918?vs=51387=51389

REVISION DETAIL
  https://phabricator.kde.org/D18918

AFFECTED FILES
  app/view/contextmenu.cpp
  app/view/view.cpp
  app/view/view.h

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18918: Make the text of "View Settings" action more specific

[Tranter Madi]

trmdi created this revision.
trmdi added a reviewer: mvourlakos.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
trmdi requested review of this revision.

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

REPOSITORY
  R878 Latte Dock

REVISION DETAIL
  https://phabricator.kde.org/D18918

AFFECTED FILES
  app/view/contextmenu.cpp
  app/view/view.cpp
  app/view/view.h

To: trmdi, mvourlakos
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart