D11333: introduce the function containmentGraphicsItemPreview

[Marco Martin]

mart updated this revision to Diff 29595.
mart added a comment.


  approach with a model
  
  a possible approach is this: a model with all the activities that map to
  items, but i don't like this, because it's pretty much duplicating the
  activity model present in kactivities with a worse version.
  
  another approach i'll try shortly, is to just have a list property
  of all possible containments and then it's a problem of the qml part to
  find the right ones for the right activities

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11333?vs=29513&id=29595

BRANCH
  mart/containmentPreview

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

AFFECTED FILES
  shell/desktopview.cpp
  shell/desktopview.h
  shell/shellcorona.cpp
  shell/shellcorona.h

To: mart, #plasma, davidedmundson
Cc: davidedmundson, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11333: introduce the function containmentGraphicsItemPreview

[Marco Martin]

mart added a comment.


  a possible approach is this: a model with all the activities that map to
  items, but i don't like this, because it's pretty much duplicating the
  activity model present in kactivities with a worse version.
  
  another approach i'll try shortly, is to just have a list property
  of all possible containments and then it's a problem of the qml part to
  find the right ones for the right activities

REPOSITORY
  R120 Plasma Workspace

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

To: mart, #plasma, davidedmundson
Cc: davidedmundson, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11333: introduce the function containmentGraphicsItemPreview

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> davidedmundson wrote in desktopview.cpp:201
> In the main code path shellcorona is responsible for assigning containments 
> to desktopviews.
> 
> In this code path DesktopView is extracting the relevant containments for 
> itself.
> 
> That's super messy, please redesign.

shellcorona assigns what is actually the current one, which is conceptually 
another thing.
not sure how to go on the other direction..
perhaps the corona could assign as well as another property like 
"allcontainmentsForScreen" which is then.. a model which has activity id, name 
and pointer to containment?

REPOSITORY
  R120 Plasma Workspace

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

To: mart, #plasma, davidedmundson
Cc: davidedmundson, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11333: introduce the function containmentGraphicsItemPreview

[Marco Martin]

mart added a comment.


  In D11333#225797 , @davidedmundson 
wrote:
  
  > ShellCorona only creates the containment in currentActivityChanged, so this 
swiping stuff isn't going to work until you've first switched containment an 
existing way, then started swiping.
  
  
  no, the Containment* instances are all created immediately, as well the 
contained ContainmentInterface*
  What's created on demand (that is heavy indeed) is the qml stuff, which is 
created when the ContainmentInterface* gets a view, which can be controlled by 
the qml part of the shell (ie, when to cann this function and reparent the 
resulting ContainmentInterface*)

REPOSITORY
  R120 Plasma Workspace

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

To: mart, #plasma, davidedmundson
Cc: davidedmundson, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11333: introduce the function containmentGraphicsItemPreview

[David Edmundson]

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


  ShellCorona only creates the containment in currentActivityChanged, so this 
swiping stuff isn't going to work until you've first switched containment an 
existing way, then started swiping.

INLINE COMMENTS

> desktopview.cpp:201
>  
> +QQuickItem *DesktopView::containmentItemForActivity(const QString &activity)
> +{

In the main code path shellcorona is responsible for assigning containments to 
desktopviews.

In this code path DesktopView is extracting the relevant containments for 
itself.

That's super messy, please redesign.

> shellcorona.cpp:1234
>  
> +Plasma::Containment *ShellCorona::containmentGraphicsItemPreview(const 
> QString& activity, int screenNum)
> +{

This is generically finding a containment.

The method name and comment shouldn't have anything to do with what 1 usage of 
it is doing.

REPOSITORY
  R120 Plasma Workspace

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

To: mart, #plasma, davidedmundson
Cc: davidedmundson, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11333: introduce the function containmentGraphicsItemPreview

[Marco Martin]

mart created this revision.
mart added a reviewer: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
mart requested review of this revision.

REVISION SUMMARY
  binded to the desktopview function containmentItemForActivity
  usable from the shell package: will be needed for gesture based
  activity switching on plasma mobile

TEST PLAN
  works well on plasma mobile

REPOSITORY
  R120 Plasma Workspace

BRANCH
  mart/containmentPreview

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

AFFECTED FILES
  shell/desktopview.cpp
  shell/desktopview.h
  shell/shellcorona.cpp
  shell/shellcorona.h

To: mart, #plasma
Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart