[Differential] [Updated] D2676: Improved and optimized Pager and Activity Pager

[hein (Eike Hein)]

hein marked 9 inline comments as done.
hein added inline comments.

INLINE COMMENTS

> davidedmundson wrote in main.qml:175
> add some readonly's over here.
> 
> It's apparently marginally faster; plus it can make things a bit more readable

Will do.

> davidedmundson wrote in main.qml:402
> why?

Using Quick's drag functionality on the delegate breaks the binding that will otherwise cause it to be repositioned based on the model's Geometry role. Resetting the model destroys and recreates the delegate. The old code relied on this happening implicitly (because that model would reset tens of thousands of times throughout a typical session on window state changes), in the new code it needs to be done explicitly. It would be possible to optimize this more, but it's not really worth more complicated code (or extra objects like Bindings) considering DND is rare and operation doesn't impact UX. I'll add a comment though.

> davidedmundson wrote in pagermodel.cpp:76
> you delete them in 
> if (!instanceCount) in the dstror,
> 
> but you're creating them statically.
> 
> that will crash on create/remove/create and it's a bit weird.

Fixed.

> davidedmundson wrote in pagermodel.cpp:84
> this is effectively duped in refreshDatasSource()

No, it's not. refreshDataSource() doesn't set anything on the models. What this lambda does is make sure that in VirtualDesktopsPager mode, the Pager always only shows windows that are on the current activity, by setting the activity filter to the current activity when it changes. Nothing in refreshDataSource() (which also isn't run when the activity changes) or refresh() (ditto) does this.

> davidedmundson wrote in pagermodel.cpp:96
> I'm a bit confused here.
> 
> If VirtualDesktop changes you're resetting your own knowledge of if the desktop is shown or not. 
> Why?

Because this is what the old code did, and I assume whoever wrote it figured out that kwin cancels show-desktop state when changing virtual desktops.

> davidedmundson wrote in pagermodel.cpp:250
> emit countChanged in both places

It's not needed in the first case because refresh() does it, but good catch on the second.

> davidedmundson wrote in pagermodel.cpp:429
> this is a new feature?
> 
> It doesn't work in the current version anyway.
> 
> Note there is a DBus verison of showDashboard which is maybe sensible to use? It cuts out some of the X specific code.

Not new feature, and works just fine for me?

REPOSITORY
  rPLASMADESKTOP Plasma Desktop

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: hein, #plasma, davidedmundson
Cc: mart, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160906/ac86fef5/attachment-0001.html>