[Differential] [Commented On] D1722: Import next-gen libtaskmanager.

[davidedmundson (David Edmundson)]

davidedmundson added a comment.


  Reviewed up to "libtaskmanager/taskfilterproxymodel.h"
  Will submit this now, in case phabricator doesn't save it.

INLINE COMMENTS

> CMakeLists.txt:86
> +endif()
> +
> +write_basic_config_version_file(${CMAKE_CURRENT_BINARY_DIR}/LibTaskManagerConfigVersion.cmake VERSION "${PROJECT_VERSION}" COMPATIBILITY AnyNewerVersion)

add_definitions(-DTRANSLATION_DOMAIN=...)

somewhere in this file

> Messages.sh:3
> +$EXTRACTRC *.ui  >> rc.cpp || exit 11
> +$XGETTEXT *.cpp */*.cpp *.h -o $podir/libtaskmanager.pot
> +rm -f rc.cpp

The legacy version is also called this.

That will cause a breakage on the i18n side.

Either: 
 two .pot files
OR
 one Messages.sh

> abstracttasksmodel.cpp:41-42
> +
> +    roles.insert(Qt::DisplayRole, "DisplayRole");
> +    roles.insert(Qt::DecorationRole, "DecorationRole");
> +

traditionally these are called "display" and "decoration" 
lowercase and without the word "Role"

Also you should be able to do

roles = QAbstractItemModel::roles()  to get them.

> activityinfo.cpp:55
> +    if (!instanceCount) {
> +        delete activityConsumer;
> +    }

..and set pointer to null

Otherwise you have a crash if you delete them all and then then create a new one.

(or use QWeak and QShared pointer )

> activityinfo.cpp:69
> +    connect(d->activityConsumer, &KActivities::Consumer::activitiesChanged,
> +        this, &ActivityInfo::numberOfActivitiesChanged);
> +}

If I have 4 activities. 2 running, 2 not.

numberOfActivities returns 2

I set one to running:

the property changes
this signal is not emitted.

> concatenatetasksproxymodel.cpp:61
> +    if (m) {
> +        // NOTE: KConcatenateRowsProxyModel offers no way to get a non-const pointer
> +        // to one of the source models, so we have to go through a mapped index.

we could add one?

sourceModelForIndex() would make sense for a few cases.

> startuptasksmodel.cpp:146
> +                    launcherUrls.insert(id.id(), launcherUrl(data));
> +                    QModelIndex idx = q->index(startups.indexOf(id));
> +                    emit q->dataChanged(idx, idx);

q->index(row)

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

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

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

To: hein, Plasma
Cc: davidedmundson, plasma-devel, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160531/faae6dbb/attachment.html>