Review Request: KDE Observatory (new plasmoid)

[Sandro Andrade]

Hi Aaron,

All suggestions have been done ! See comments below.

Thanks,
Sandro

> * i see that it uses Qwt in one class. this is likely to diminish both the
> user base of it as well as it's "plasma-ness". does Plasma::SignalPlotter give
> you something similar? if not, then a check in the CMakeLists.txt file for Qwt
> is required

I've tried the SignalPlotter but apparently its time-dependent
approach doesn't properly fit the needs in commit history. I need to
include data which are sparse in time and that would be harder to
achieve. So, I keep using Qwt, cmake module was included, and checked
in CMakeLists.txt.

> * the collectors really look like something that would be better as a
> DataEngine. putting data and visualization into one plugin is generally
> frowned upon in plasma. things like the periodic updates of the collectors are
> taken care of for you if you use a DataEngine, other widgets can more easily
> use the results, the data can be shared between multiple instances of the
> kdeobservatory widget and the widget would be more ready for remote widget
> use. perhaps not necessary for 4.4, but should definitely be considered for
> 4.5.

Postponed to 4.5.

> * m_progressProxy should be a Plasma::Meter (i just made some relevant methods
> slots in Plasma::Meter to make that even easier)

Done !

> * collectFinished is probably getting called more often than needed as its
> connected to multiple collectors; that means views and what not will get
> updated for collectors that don't have new data?

Views are updated only when all collectors finish.

> * calling collection[something] as in "if (pair.second &&
> m_viewProviders[view])" will create an entry in the collection if it doesn't
> exist which is probably an unwanted (or at least unneeded) side effect; in
> these cases you probably want m_viewProviders.value(view)

Done !

> * passing in a rect and the code in IViewProvider::createView looks fishy and
> indeed doesn't quite work; when KdeObservator::m_viewContainer changes
> geometry, the views won't change their geometry. i'd suggest either using a
> QGraphicsLayout or else putting an event filter on the view container and
> adjusting the geometry of the views when the view container gets a resize or
> related events.

Done ! Applet was converted to a PopupApplet and all geometry
adjustments are done when resizing events occur.

> * calls like m_right->nativeWidget()->setIcon are generally unecessary. use
> m_right->setIcon() i'd look through all the nativeWidget() calls and see which
> can be removed.

Done !

> * on first update, it sat and spun for the better part of a minute writing to
> disk. subsequent starts up are good. i wonder if this is because each commit
> is added one at a time to the sqllite db instead of several at once? this is a
> show stopper for first use in any case. it also happens when adding more days
> to the context :/

Solved by initiating a transaction before adding commits in database
and committing when everything has finished. There are no delays after
this
change.

> * worse, it blocks the app while doing so. the db should either be updated
> incrementally with regular returns to the mainloop to prevent blocking the
> whole plasma application (e.g. plasma-desktop) or it should be done in a
> thread (probably preferred in this case)

Also solved by previous change. No blocking anymore.

> * the icon button in the Configure KDE Observatory page seems to have some
> layout issues; here it overlaps the divider line at the bottom?

Fixed !

> * sync'ing every minute might be a bit much for the default. 5 or 10 minutes
> should be plenty for a default?

Done !

> * speaking of defaults, is showing just one day of operations really enough to
> give a decent and moral boosting overview? would the last week's worth of data
> be better? a lot of projects see sporadic but regular commits and "one day"
> may not be a whole lot of progress. plasma is active daily as are some of the
> other big projects like kdepim ... but not all are so lucky. this does make
> the db building problem worse, of course.

Default value adjusted to 7 days. Although app blocking doesn't occur
anymore, some time (around 2.5 min) is required for the initial load
of seven-day commits. After that, updates occur very fast.

> * why is "enable transition effects" an option?

I've kept this as an option because disabling effects makes view
transition faster.

> * why are some view options in the global config options and others in the
> Views page?

All view configuration options were grouped in Views configuration page.

> * from a user's perspective, why is there a "global config options"? would
> another name be more useful?

Renamed to "General".

> * the automatic transitions are nice, but when trying to page through them
> with the left/right buttons it really gets in the way. perhaps a "play/pause"
> button could be added to the interface and automatically set it to "pause"
> when one of the arrow buttons are manually pressed? perhaps the transition
> could also pause when the mouse is placed over a graph; this would make the
> tooltips more easily reachable, as right now getting tooltips is a bit of a
> challenge when the animations are ongoing.

Added a longer pause when user clicks left/right buttons.
When mouse is placed over a block in any view, transitions are paused
until tooltip disappears and mouse leaves block area.

> * it seems that whenever the data is updated, the "slide show" starts from the
> beginning again; it looks a bit odd to have it start all over again. could it
> simply keep going with the slideshow instead of restarting it?

That would be harder to implement since changing configurations often implies
in views being created or removed, making views tracking a little bit difficult.
Anyway, I think configuration operations aren't going to be quite often.

> * the scroll arrows feel "backwards": the right arrow should take me to the
> "next" item, which means things should scroll to the left not the right.

Fixed !

> * it would be really nice if it were possible to aggregate some project data;
> e.g. "plasma" is really kdelibs/plasma + kdebase/runtime/plasma +
> kdebase/workspace/plasmas + kdeplasma-addons ... not a required feature for
> sure, but a nice one to have perhaps

Fixed. If some project commit subject is "plasma" all commits involving any
plasma branch will be considered. Additionally, Krazy collector was improved
to aggregate data from several apps inside a module, so you can
visualize all krazy
errors in KDE-edu for example.

> * it would make the widget much more immediately approachable if there were
> some project presets shipped, such as ones for kdelibs, kdepim, kdegames,
> plasma, etc. do you have thoughts on how that would be best achieved?

Presets were created to almost all KDE main applications. See
kdeobservatorypresets.cpp. These are loaded at first run, and can be
removed, edited. New projects can also be added as previously described.