D13880: [KMoreTools] Reduce menu hierarchy

2018-09-15 Thread Nicolas Fella 
This revision was automatically updated to reflect the committed changes.
Closed by commit R304:9f54611a472d: [KMoreTools] Reduce menu hierarchy 
(authored by nicolasfella).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13880?vs=41581=41716

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

AFFECTED FILES
  src/kmoretools/kmoretools.cpp

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-09-15 Thread Dominik Haumann 
dhaumann accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R304 KNewStuff

BRANCH
  arcpatch-D13880

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-09-13 Thread Nicolas Fella 
nicolasfella updated this revision to Diff 41581.
nicolasfella added a comment.


  - Address comments

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13880?vs=41575=41581

BRANCH
  arcpatch-D13880

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

AFFECTED FILES
  src/kmoretools/kmoretools.cpp

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-09-13 Thread Dominik Haumann 
dhaumann added inline comments.

INLINE COMMENTS

> kmoretools.cpp:569
> +if (!mstruct.notInstalledServices.isEmpty()) {
> +qDebug() << "notInstalledItems not empty => build 'Not 
> installed' section";
> +parent->addSection(i18nc("@action:inmenu", "Not installed:"));

Ah, and please comment out this qDebug() statement again - it was commented out 
before as well :-)

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-09-13 Thread Dominik Haumann 
dhaumann added a comment.


  Almost good enough, can you remove the forward declaration again? Just 
another one-line change. I then don't have any objections anymore.

INLINE COMMENTS

> kmoretools.h:536
>  class KMoreToolsMenuBuilderPrivate;
> +class KmtMenuStructure;
>  

Please remove also this line again, then kmoretools.h remains completely 
unchanged.

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-09-13 Thread Nicolas Fella 
nicolasfella updated this revision to Diff 41575.
nicolasfella added a comment.


  - Move method to private

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13880?vs=37136=41575

BRANCH
  arcpatch-D13880

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

AFFECTED FILES
  src/kmoretools/kmoretools.cpp
  src/kmoretools/kmoretools.h

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-09-13 Thread Nathaniel Graham 
ngraham added a comment.


  @nicolasfella ping! It would be a shame to lose this since it's already quite 
a good improvement.

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-08-13 Thread Dominik Haumann 
dhaumann added a comment.


  Ok - then with minor changes, let's move this forward. Who is going to do the 
work? :-)

INLINE COMMENTS

> kmoretools.cpp:650
> +if (mstruct.mainItems.isEmpty()) {
> +createMoreMenu(mstruct, menu);
> +} else {

This line then changes to d->createMoreMenu(...);

> kmoretools.h:664
> + */
> +void createMoreMenu(const KmtMenuStructure , QMenu *parent);
> +

Please move to .cpp file behind the d-pointer.

Reasoning: We usually do not expose private functions in public interafces. 
This has slipped for this class in the past it seems, so for KF6, this needs to 
be changed anyways. But let's start now: move createMoreMenu() into the cpp 
file.

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-08-13 Thread Nathaniel Graham 
ngraham added a comment.


  I'm okay with this patch as a first step, FWIW.

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-08-13 Thread Nicolas Fella 
nicolasfella added a comment.


  In D13880#307597 , @dhaumann wrote:
  
  > What is the current state of this?
  >  The new private functions should move to the pimpl class behind the 
d-pointer.
  >
  > In general: Looking at the public classes, /all/ the private functions 
should have been hidden behind the pimpl class...
  
  
  We agreed on how we want to structure the menu, but I haven't had time to 
implement it. IMHO it is out of scope for this patch, though. I think this 
patch is still an improvement for cases where aforementioned redesign won't be 
used and worth merging (after I address your comment)

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-08-13 Thread Dominik Haumann 
dhaumann added a comment.


  What is the current state of this? The new private functions should move to 
the pimpl class behind the d-pointer.
  
  In general: Looking at the public classes, /all/ the private functions should 
have been hidden behind the pimpl class...

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-07-06 Thread gregormi 
gregormi added a comment.


  The spirit goes in the right direction; please proceed :-). Note, that Kate's 
project plugin also uses KMoreTools.

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-07-06 Thread Nathaniel Graham 
ngraham added a comment.


  Sounds good to me! It sounds like we're all in agreement?

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-07-05 Thread Dominik Haumann 
dhaumann added a comment.


  I like the idea as well. +1

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-07-05 Thread Nicolas Fella 
nicolasfella added a comment.


  In D13880#287481 , @gregormi wrote:
  
  > +1 for your suggestions. Downside: higher implementation effort.
  
  
  I like it, too, and I am willing to put effort in it, as soon as we agree on 
a way forward.
  
  > A bit of history: When I was creating the More submenu and the Configure... 
item I was pondering a thing: I was (and still am) not so glad about those two 
items because they are seldom used but always present in the menu. Back then, I 
thought about adding a keyboard modifier (like the Shift key) which must be 
pressed to let the Configure... item appear (similar to the context menu in 
Windows Explorer where the Shift key let more advanced items appear like Run as 
admin... etc.). I discarded the idea because of its hard discoverability.
  > 
  > Another alternative would be to somehow hook the Configuration of 
KMoreTools menus in the host applications main Configure... dialog. Downside: 
configuration is far away from the actual menu.
  > 
  > It would be cool if instead of the text "Configure..." or "More" there 
would be a small gear icon that does not take the space of a whole menu item. I 
suspect this is not possible with QMenus.
  
  I think this is quite orthogonal to the hierarchy discussion.
  
  @ngraham Your approach makes total sense for Spectacle, but would not be 
appropriate for e.g. the disk utility or find menu  in Dolphin since there is 
no menu level above to embed the actions. It's no big issue, just something we 
need to take into account when implementing.
  
  As for the implementation my idea would be:
  Additionally to the full menu we expose the top-level applications/more menu 
as List of Actions/Menu. Then the calling applications can embed them in any 
way they want.
  
  With this approach this patch would still be an improvement for cases where 
the old menu is used, e.g. the Dolphin cases I described earlier

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-07-05 Thread gregormi 
gregormi added a comment.


  > You are the most awesome person in the world today.
  
  +1 :-)
  
  > Pro: Very flat hierarchy, code could be simplified a lot
  >  Con: Unwanted and uninstalled tools would be present all the time.
  
  Exactly my thoughts. The idea behind the More menu was to hide (a potentially 
large number) of alternative programs which are not installed and not needed 
for the user. And even if there is only one alternative not installed program 
which the user does not want to use, always showing a "Install XYZ" seems a bit 
obtrusive to me.
  
  > https://phabricator.kde.org/D13880#286937
  
  +1 for your suggestions. Downside: higher implementation effort.
  
  A bit of history: When I was creating the More submenu and the Configure... 
item I was pondering a thing: I was (and still am) not so glad about those two 
items because they are seldom used but always present in the menu. Back then, I 
thought about adding a keyboard modifier (like the Shift key) which must be 
pressed to let the Configure... item appear (similar to the context menu in 
Windows Explorer where the Shift key let more advanced items appear like Run as 
admin... etc.). I discarded the idea because of its hard discoverability.
  
  Another alternative would be to somehow hook the Configuration of KMoreTools 
menus in the host applications main Configure... dialog. Downside: 
configuration is far away from the actual menu.
  
  It would be cool if instead of the text "Configure..." or "More" there would 
be a small gear icon that does not take the space of a whole menu item. I 
suspect this is not possible with QMenus.

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nathaniel Graham 
ngraham added a comment.


  If it's important for uninstalled tools to be not always be visible, maybe 
what we should do is embed the //content// of the KNewStuff menu in Spectacle 
rather than exposing the whole thing as a sub-menu. Then we could make a few 
string changes, and it would be like this in Spectacle:
  
  No apps installed:
  
Open Screenshots folder
Print...
Install apps to Record screen>
  
  One app installed:
  
Open Screenshots folder
Print...
--
 Record screen
Open SimpleScreenRecorder
Install alternatives >   Peek
 Vokoscreen
  
  All apps installed:
  
Open Screenshots folder
Print...
--
 Record screen
Open SimpleScreenRecorder
Open Peek
Open Vokoscreen
  
  This would have the benefit that you could avoid a level of hierarchy for the 
workflow of actually actually opening an installed screen recording app in 
Spectacle, but would have the disadvantage that it would involve some 
additional code changes in Spectacle (and other apps that include the KNewStuff 
menu as a sub-menu).

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nicolas Fella 
nicolasfella added a comment.


  In D13880#286816 , @ngraham wrote:
  
  > Or even this:
  >
  >   Open SimpleScreenRecorder
  >   -
  >   Install Peek
  >   Install Vokoscreen
  >   -
  >   Configure...
  >
  
  
  Put into code it would look like
  
  F6013983: Screenshot_20180704_172717.png 

  
  Pro: Very flat hierarchy, code could be simplified a lot
  Con: Unwanted and uninstalled tools would be present all the time.
  
  Before I proceed with any idea I would like to have an opinion from @gregormi 
or @dhaumann

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nicolas Fella 
nicolasfella added a comment.


  In D13880#286820 , @ngraham wrote:
  
  > In D13880#286818 , @nicolasfella 
wrote:
  >
  > > Do you know if/how we can check if there is a handler for appstream://? 
So we can fall back to website if none is available
  >
  >
  > I think you added that data and functionality in D13706 
, no? Looks like you can just try to get 
`appstreamId`, and if you get back an empty string, then no appstream URL is 
defined for it and you can fall back to the homepage.
  
  
  No, what I mean is check if Discover or any app that handles appstream:// is 
available. The install action is pretty useless without an application handling 
it.

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nathaniel Graham 
ngraham added a comment.


  In D13880#286818 , @nicolasfella 
wrote:
  
  > Do you know if/how we can check if there is a handler for appstream://? So 
we can fall back to website if none is available
  
  
  I think you added that data and functionality in D13706 
, no? Looks like you can just try to get 
`appstreamId`, and if you get back an empty string, then no appstream URL is 
defined for it and you can fall back to the homepage.

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nicolas Fella 
nicolasfella added a comment.


  In D13880#286800 , @ngraham wrote:
  
  > You are the most awesome person in the world today.
  
  
  Yay!
  
  > Here's a radical thought: get rid of the More menu entirely and move all of 
its content inline, like so:
  
  Sound good to me
  
  > And clicking on the menu items for any of the Install options would open 
the appstream:// URL, falling back to the home page if no AppStream information 
is available. I think that would be super slick.
  
  That would have been my next patch. Do you know if/how we can check if there 
is a handler for appstream://? So we can fall back to website if none is 
available

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nathaniel Graham 
ngraham added a comment.


  Or even this:
  
Open SimpleScreenRecorder
-
Install Peek
Install Vokoscreen
-
Configure...
  
  ...And clicking on the menu items for any of the Install options would open 
the `appstream://` URL, falling back to the home page if no AppStream 
information is available. I think that would be super slick.

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nathaniel Graham 
ngraham added a comment.


  You are the most awesome person in the world today.
  
  Here's a radical thought: get rid of the More menu entirely and move all of 
its content inline, like so:
  
  Installed:
SimpleScreenRecorder
-
Not installed:
Peek   >
Vokoscreen >
-
Configure...

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nicolas Fella 
nicolasfella edited the test plan for this revision.

REPOSITORY
  R304 KNewStuff

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

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13880: [KMoreTools] Reduce menu hierarchy

2018-07-04 Thread Nicolas Fella 
nicolasfella created this revision.
nicolasfella added reviewers: gregormi, dhaumann, ngraham.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
nicolasfella requested review of this revision.

REVISION SUMMARY
  Skip 'More' submenu and attach its children directly to the menu when 'More' 
is the only submenu.

REPOSITORY
  R304 KNewStuff

BRANCH
  hidemore

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

AFFECTED FILES
  src/kmoretools/kmoretools.cpp
  src/kmoretools/kmoretools.h

To: nicolasfella, gregormi, dhaumann, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns