Details

Reviewers

ervin
franckarrecot

Commits

R4:7ec90d075379: New feature: running task widget

Summary

Start a task and a small line appears at the top of your desktop.
Moving the mouse onto that line expands to a bar that shows

[Stop]  Task title   [Done],

as a reminder of what the current task is.

Design (new classes) :

RunningTaskModel to store the running task (with base interface for type safety of stubs).
RunningTaskWidget and PageView on top of that model
TaskApplicationModel to create the RunningTaskModel with its deps
TaskApplicationComponents to connect PageView and RunningTaskWidget to RunningTaskModel

Test Plan

No cucumber tests because OpenSuSE botched the cucumber packaging.

(https://bugzilla.opensuse.org/show_bug.cgi?id=1015940)

Unit tests for all new classes though, and for serialization of the 'running' attribute.

Diff Detail

Repository

R4 Zanshin

Branch

pomodoro

Lint

No Linters Available

Unit

No Unit Test Coverage

dfaure updated this revision to Diff 7334.Oct 12 2016, 4:46 AM

dfaure retitled this revision from to First implementation of a pomodoro widget: start a task and see its title at the top of your desktop..

dfaure edited the test plan for this revision. (Show Details)

dfaure added reviewers: ervin, franckarrecot.

dfaure added a subscriber: Zanshin.

ervin set the repository for this revision to R4 Zanshin.Nov 23 2016, 8:57 AM

ervin requested changes to this revision.Nov 23 2016, 6:39 PM

ervin edited edge metadata.

ervin added inline comments.

src/widgets/pageview.cpp
377	Unneeded change
src/widgets/pomodorowidget.cpp
37 ↗	(On Diff #7334)	Might be why krunner is below it?
41 ↗	(On Diff #7334)	I'd say yes.
src/widgets/pomodorowidget.h
35 ↗	(On Diff #7334)	As discussed could use a rename.
44 ↗	(On Diff #7334)	Guess it'll change once the stop action is here and not in the main window anymore?

This revision now requires changes to proceed.Nov 23 2016, 6:39 PM

A number of GUI changes discussed with Kévin.

The auto-hide behaviour will be done in a separate patch, this is big enough already, and usable as is.

ervin edited edge metadata.Dec 2 2016, 5:01 PM

ervin set the repository for this revision to R4 Zanshin.

I guess the first comment will lead to bigger changes than anticipated. It likely means comparing if two tasks are identical and we just got pointers, we try to limit the number of instances a task can have in memory but it's not guaranteed to be a single one. Means we'd need to have ids to compare to, we could, but we don't expose that in the domain yet. That's something which could be done in a separate commit introducing id() on Artifact, that one would build up on it.

src/domain/task.h
36	Hm, it's purely something for views, I'd rather have the concept out of the domain and into widgets instead. Either by introducing a wrapper type in the widgets module, or having the ApplicationComponents or PageView know which task is the running task?
src/widgets/runningtaskwidget.cpp
46–47	Probably wants to have that ifdef'able depending on KWindowSystem availability. Otherwise it's going to bite us on Windows/Mac when we finally get there.
52	But but but, I wanted it fuschiaaaa!!!! ;-)
src/widgets/runningtaskwidget.h
43	Please move implementation in cpp file.

ervin requested changes to this revision.Dec 2 2016, 5:19 PM

ervin edited edge metadata.

This revision now requires changes to proceed.Dec 2 2016, 5:19 PM

dfaure added inline comments.Dec 11 2016, 9:53 AM

src/domain/task.h
36	OK, I was wondering about that. It sure was convenient (centralized and with notification)... ;) I don't think a wrapper type is a good idea, we only have one running task at any given point. My problem is synchronizing this knowledge between PageView and RunningTaskWidget, when both can change that fact. So option 1 is a bunch more signal/slot connections between the two (via ApplicationComponents), option 2 is a RunningTaskController which centralizes the knowledge for these two widgets. Seems like a better idea, and I guess it would fit in "presentation" ? I'm just afraid you're going to make me mock it via all sorts of un-type-safe QObject * and properties like I see in the rest of the code ;-) (personally I would unittest PageView and RunningTaskWidget with a proper RunningTaskController behind, to avoid missing something due to a mock)
src/widgets/runningtaskwidget.cpp
46–47	The KWindowSystem API seems to be available on all platforms (Windows/Mac are mentionned in the yaml file, no ifdefs in the headers, nor in the CMakeLists.txt for installing headers). Whether it actually does anything at runtime is another story (I see a plugin system and a Dummy class as fallback, which does nothing). But I think this means this code is OK.
52	Change your selection background color to fuschia in `kcmshell5 colors` :)

ervin added inline comments.Dec 19 2016, 9:23 PM

src/domain/task.h
36	You guessed right, we're going through properties between presentation and widgets. I swear there's a reason for it, it's named QML. Widgets won't be the only use for the presentations. The untype safe QObject* could disappear though, it's a left-over from Qt4, now it's dealt with properly in Qt5.
src/widgets/runningtaskwidget.cpp
46–47	OK then.

Reworked design, implemented serialization of 'running' attribute.

dfaure retitled this revision from First implementation of a pomodoro widget: start a task and see its title at the top of your desktop. to New feature: running task widget.Jan 17 2017, 11:24 PM

dfaure updated this object.

dfaure edited the test plan for this revision. (Show Details)

dfaure edited edge metadata.

Getting there, I'm looking forward to see this merged. :-)

src/akonadi/akonadiserializer.cpp
281	After looking at status again, there's only one status at a time and it's not orthogonal with done. Also it seems suited for the workflow feature I'd like to introduce at some point. Could you switch to a custom property instead? With our design it should impact only the serializer related part of your patch.
src/presentation/runningtaskmodel.cpp
35	Better as part of the initialization list
47	ditto
63	error handling missing here, will require inheriting from ErrorHandlingModelBase like PageModel and also calling setErrorHandler at the creation point. Probably RunningTaskModelInterface should be the one inheriting ErrorHandlingModelBase.
68	ditto
75	Unnecessary AFAICT. Since setRunningTask(nullptr) will get in the first if anyway.
src/presentation/runningtaskmodel.h
38	nitpick: remove this empty line please.
src/presentation/runningtaskmodelinterface.h
38	nitpick: please remove this empty line
src/presentation/taskapplicationmodel.cpp
43	That would be where you call setErrorHandler
src/presentation/taskapplicationmodel.h
37	nitpick: Remove that empty line please
src/renku/kontact/part.cpp
39 ↗	(On Diff #10305)	Renku shouldn't need this change. It's the note application, it doesn't show any task.
59 ↗	(On Diff #10305)	ditto
src/widgets/taskapplicationcomponents.cpp
49	You create this unconditionally in the setter, this check is not necessary I think.
src/zanshin/app/main.cpp
55–59	Please do that in dependencies.cpp The includes of dependencymanager.h and runningtaskmodel.h above won't be necessary then
tests/units/presentation/runningtaskmodeltest.cpp
146–153	I'd prefer if such state could be avoided. This way we make sure we have no unwanted dependency between tests.
tests/units/widgets/runningtaskwidgettest.cpp
50–67	Please move those in the definition of the stub class

This revision now requires changes to proceed.Jan 24 2017, 5:59 PM

Apply changes requested by Kévin

dfaure marked 19 inline comments as done.Feb 10 2017, 7:46 AM

dfaure added inline comments.

src/akonadi/akonadiserializer.cpp
281	The fact that it's not orthogonal with done wasn't an issue, you can be running a done task. But OK, if you want to use status for something else, I switched to a custom property, it's not like this is really interoperable with anything else.
src/presentation/runningtaskmodel.h
38	Strange, for me it's logical to have an empty line before every "public:", "protected:" etc. -- including the first one, when there's stuff before it.
src/renku/app/org.kde.renku.appdata.xml
55	Erg, this isn't actually part of my change. But I rebased, I guess that's what leads to this. Oops.
tests/units/presentation/runningtaskmodeltest.cpp
146–153	Well these vars didn't really have state, in theory. But OK, moved them to a class I instanciate every time. Better ? I certainly don't want to just copy/paste 10 lines in every method.

A few more nitpicks and changes due to the newer code in there and me being stupid not seeing a couple of things on previous rounds *sigh*.

src/domain/task.cpp
106	Damn... I realize only now that it's missing a if (m_running == running) return; at the beginning of that method.
src/domain/task.h
89	No need to keep that comment, as soon as someone connect to it or binds to the property from QML (thinking far ahead) it'll get used. Just pointing out that for signals this can change quickly.
src/renku/app/org.kde.renku.appdata.xml
55	OK, weird, still seeing some of it, in the desktop files as well. I'll assume it's not in your change indeed. I don't see you changing those translations anyway. ;)
src/widgets/runningtaskwidget.h
45	Q_NULLPTR
tests/units/presentation/runningtaskmodeltest.cpp
40–41	nitpicks: break line before {, no empty line at the start of the block
146–153	Yes, that's a better way of proceeding, either an helper type like you did or a factory method.
tests/units/widgets/runningtaskwidgettest.cpp
36	nitpick: break before {
42	ditto
45	empty line after that one
46	ditto

This revision now requires changes to proceed.Feb 25 2017, 9:15 PM

coding style fixes

ervin accepted this revision.Feb 27 2017, 7:14 AM

This revision is now accepted and ready to land.Feb 27 2017, 7:14 AM

dfaure closed this revision.Feb 27 2017, 8:24 AM

		Path
M		CMakeLists.txt (1 line)
M		src/akonadi/akonadiserializer.cpp (6 lines)
M		src/domain/task.h (5 lines)
M		src/domain/task.cpp (14 lines)
M		src/presentation/CMakeLists.txt (3 lines)
A	M	src/presentation/runningtaskmodel.h (62 lines)
A	M	src/presentation/runningtaskmodel.cpp (82 lines)
A	M	src/presentation/runningtaskmodelinterface.h (58 lines)
A	M	src/presentation/runningtaskmodelinterface.cpp (12 lines)
A	M	src/presentation/taskapplicationmodel.h (50 lines)
A	M	src/presentation/taskapplicationmodel.cpp (47 lines)
M		src/renku/app/org.kde.renku.appdata.xml (6 lines)
M		src/renku/app/org.kde.renku.desktop (2 lines)
M		src/widgets/CMakeLists.txt (3 lines)
M		src/widgets/applicationcomponents.h (11 lines)
M		src/widgets/applicationcomponents.cpp (7 lines)
M		src/widgets/pageview.h (13 lines)
M		src/widgets/pageview.cpp (63 lines)
A	M	src/widgets/runningtaskwidget.h (76 lines)
A	M	src/widgets/runningtaskwidget.cpp (138 lines)
A	M	src/widgets/taskapplicationcomponents.h (51 lines)
A	M	src/widgets/taskapplicationcomponents.cpp (66 lines)
M		src/zanshin/app/dependencies.cpp (5 lines)
M		src/zanshin/app/main.cpp (8 lines)
M		src/zanshin/app/org.kde.zanshin.appdata.xml (6 lines)
M		src/zanshin/app/org.kde.zanshin.desktop (2 lines)
M		src/zanshin/app/zanshinui.rc (4 lines)
M		src/zanshin/kontact/part.cpp (8 lines)
M		tests/units/akonadi/akonadiserializertest.cpp (44 lines)
M		tests/units/presentation/CMakeLists.txt (2 lines)
A	M	tests/units/presentation/runningtaskmodeltest.cpp (170 lines)
A	M	tests/units/presentation/taskapplicationmodeltest.cpp (70 lines)
M		tests/units/widgets/CMakeLists.txt (2 lines)
M		tests/units/widgets/pageviewtest.cpp (70 lines)
A	M	tests/units/widgets/runningtaskwidgettest.cpp (175 lines)
A	M	tests/units/widgets/taskapplicationcomponentstest.cpp (89 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	7334	4275cf8		Oct 12 2016, 4:46 AM	★	★
Diff 2	8688	a8f6e81	A number of GUI changes discussed with Kévin.	Dec 1 2016, 9:46 PM	★	★
Diff 3	10305	056987b	Reworked design, implemented serialization of 'running' attribute.	Jan 17 2017, 11:22 PM	★	★
Diff 4	11145	d86ce5b	Apply changes requested by Kévin	Feb 10 2017, 7:40 AM	★	★
Diff 5	11851	d86ce5b	coding style fixes	Feb 26 2017, 1:48 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
8e2f4d429f3d	8264b64b148b	08a30d576415	David Faure	New feature: running task widget (Show More…)	Feb 26 2017, 1:48 PM
08a30d576415	35ac11074d96	09bae68dbd38	l10n daemon script	SVN_SILENT made messages (.desktop file) - always resolve ours (Show More…)	Feb 3 2017, 5:16 AM
09bae68dbd38	8fcf77c2c6ed	d86ce5bed51b	l10n daemon script	GIT_SILENT made messages (after extraction)	Feb 3 2017, 3:19 AM

New feature: running task widgetClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 11851

CMakeLists.txt

src/akonadi/akonadiserializer.cpp

src/domain/task.h

src/domain/task.cpp

src/presentation/CMakeLists.txt

src/presentation/runningtaskmodel.h

src/presentation/runningtaskmodel.cpp

src/presentation/runningtaskmodelinterface.h

src/presentation/runningtaskmodelinterface.cpp

src/presentation/taskapplicationmodel.h

src/presentation/taskapplicationmodel.cpp

src/renku/app/org.kde.renku.appdata.xml

src/renku/app/org.kde.renku.desktop

src/widgets/CMakeLists.txt

src/widgets/applicationcomponents.h

src/widgets/applicationcomponents.cpp

src/widgets/pageview.h

src/widgets/pageview.cpp

src/widgets/runningtaskwidget.h

src/widgets/runningtaskwidget.cpp

src/widgets/taskapplicationcomponents.h

src/widgets/taskapplicationcomponents.cpp

src/zanshin/app/dependencies.cpp

src/zanshin/app/main.cpp

src/zanshin/app/org.kde.zanshin.appdata.xml

src/zanshin/app/org.kde.zanshin.desktop

src/zanshin/app/zanshinui.rc

src/zanshin/kontact/part.cpp

tests/units/akonadi/akonadiserializertest.cpp

tests/units/presentation/CMakeLists.txt

tests/units/presentation/runningtaskmodeltest.cpp

tests/units/presentation/taskapplicationmodeltest.cpp

tests/units/widgets/CMakeLists.txt

tests/units/widgets/pageviewtest.cpp

tests/units/widgets/runningtaskwidgettest.cpp

tests/units/widgets/taskapplicationcomponentstest.cpp

New feature: running task widget
ClosedPublic
Actions

Revision Contents
Changeset List