Adjustable Clock, Spell Check and Window List applets moved to kdereview

[Emdek]

On 04-10-2009 at 21:49:04 Aaron J. Seigo <aseigo at kde.org> wrote:
> On October 4, 2009, Emdek wrote:
>> Adjustable Clock is a clock that uses WebKit to render advanced date and
>> time formats (maybe it need some refactoring of configuration dialog,
>> mostly hiding some not really needed or too advanced options).
>
> this one is perhaps the hardest of the three to decide what to do with;  
> it
> uses libplasmaclock, but that library has no binary compatibility  
> guarantees
> on it. that makes plasmoids which link to libplasmaclock that exist  
> outside of
> kdebase or addons really risky. (we already have bug reports that get  
> filed
> about this exact issue :/)

Yes, this can cause problems (especially when trying to change order of  
default pages...) but copying it and updating it every time (mostly to  
have consistent configuration interface) is also bad idea...


> using webkit to render a clock is a bit overkill perhaps, but as an  
> optional
> thing it's probably just fine. the options it offers are, however, really
> rather overkill (as you note).

It was using plain label before, and for sure, it is really not enough to  
get interesting results (not everybody likes plain clocks). :-)
And in my case it is not using more resources than label but gives much  
bigger possibilities (mostly advanced CSS options).
I was thinking about adding an option to select "rendering engine" (could  
be determined when saving layout, when it find in the code something  
untypical then it switch to WebKit for example) but it would make it more  
complex and has no advantages...


> why a time difference option when timezones are available? "paint  
> background"
> is an option we intentionally do not provide in individual plasmoids, and
> scaling content should be a given not an option.

Time difference was added for other reasons that are no longer valid and  
it will be simply removed.
Option to disable background painting is different story. The problem is  
that some formats looks better without it. Maybe it could be at least  
hidden option?
Content scaling is an option because it was experimental (zooming  
capabilities were added in Qt 4.5 as far as I remember) but it looks like  
it works correctly now.


> it's an interesting widget, no doubt about it, but i think it could use  
> a few
> rounds of polishing before being included in an official release. i'm  
> not sold
> on the name, either, of "Adjustable Clock" since it doesn't really speak  
> to
> what gets adjusted :)

There could be problem with changing it's name, it is available since one  
year and it is now available in some repositories...
And how it could be named? I've chosen this name because I wanted to have  
clock that could be adjusted as much as possible (but not too much). ;-)



>> Spell Check is applet for fast spell checking (dialog could be ported to
>> QGraphicsView when bug with embedded pop ups will be fixed in Qt).
>
> nice idea; would make a nice addition to addons, imo. when using it, i  
> was
> surprised it picked what i had copied with Ctrl+C. perhaps it would make  
> more
> sense to pick whatever is on the selection clipboard rather than the
> copy/paste clipboard? this would allow one to just highlight some text  
> and
> then hit the spell check button. copy/paste would still be easily  
> possible
> with ctrl+v.

Yes, it is good idea.
It could check if there is selection in clipboard and if not then try to  
find copied text (backward compatibility).


> when is m_dialog deleted?

It's slot deleteLater() is connected to destroyed() signal emitted from  
applet.


> could m_dialog be created not in init() but when it's about to be shown?  
> would
> help speed up startup a bit and lower overhead when not used.

Yes, good idea. :-)



>> Window List is menu with list of opened windows (similar to applet for  
>> KDE
>> 3.5).
>
> what is the use case for the useArrowIcon hidden config item? and if it  
> is to
> remain, it will need to be adjusted in a constraintsEvent implementation  
> on
> LocationConstraint changes.

That option was added because many users wanted to have "old style" icon  
(like in KDE 3.5).
If you want, I can simply remove it or add that improvement.


> otherwise, if you're ok with changing the coding style to the kdelibs  
> style
> this one could probably go into  
> kdebase/workspace/plasma/desktop/applets/ as-
> is.

This is not a problem, my coding style is not so much different (but a bit  
more readable for me ;-)).


Thanks for review, I'll try to made needed changes during this weekend.