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

[davidedmundson (David Edmundson)]

davidedmundson added a comment.


  Generally awesome.
  One major bug, rest are tiny comments.
  
  Will give it a second review later, as it's a bit patch and I got tired :)

INLINE COMMENTS

> main.qml:175
>  
> -            mainText: desktopName
> -            // our ToolTip has maximumLineCount of 8 which doesn't fit but QML doesn't
> -            // respect that in RichText so we effectively can put in as much as we like :)
> -            // it also gives us more flexibility when it comes to styling the <li>
> -            textFormat: Text.RichText
> +        property int effectiveRows: {
> +            var columns = Math.floor(repeater.count / pagerModel.layoutRows);

add some readonly's over here.

It's apparently marginally faster; plus it can make things a bit more readable

> main.qml:402
> +
> +                                        pagerModel.refresh();
> +                                    } else {

why?

> pagermodel.cpp:76
> +int PagerModel::Private::instanceCount = 0;
> +ActivityInfo *PagerModel::Private::activityInfo = new ActivityInfo();
> +VirtualDesktopInfo *PagerModel::Private::virtualDesktopInfo = new VirtualDesktopInfo();

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.

> pagermodel.cpp:84
> +
> +    QObject::connect(activityInfo, &ActivityInfo::currentActivityChanged, q,
> +        [this]() {

this is effectively duped in refreshDatasSource()

> pagermodel.cpp:96
> +        [this]() {
> +            desktopDown = false;
> +        }

I'm a bit confused here.

If VirtualDesktop changes you're resetting your own knowledge of if the desktop is shown or not. 
Why?

> pagermodel.cpp:250
> +        d->windowModels.clear();
> +
> +        endResetModel();

emit countChanged in both places

> pagermodel.cpp:429
> +        // Toggle the desktop.
> +        if (d->showDesktop) {
> +            NETRootInfo info(QX11Info::connection(), 0);

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.

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: 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/0c078e70/attachment.html>