Review Request: [Quicklaunch] Refactoring of the Quicklaunch applet

[Ingomar Wesp]

> On None, Lukas Appelhans wrote:
> > Hey!
> > 
> > Great work! I had a look over the code and tested it and it works greatly!
> > 
> > 2 things:
> > Why do we use a custom layout instead of a QGridLayout?
> > And I think the column setting is unnecessary... no? :)
> > 
> > Anyway, as the freeze is near, I added an entry to the feature plan about this...
> > 
> > I would say +1 for committing after the 2 things have been discussed...
> > 
> > Lukas

Hi!

> Great work! I had a look over the code and tested it and it works greatly!
Thanks, I'm happy to hear that.

> Why do we use a custom layout instead of a QGridLayout?

2 reasons, primarily.
For one, I didn't want to have to completely repopulate the underlying QGraphicsGridLayout every time the icon wrapping changes (caused by resizes or changes to the applet's configuration). Secondly, I thought that the code is simpler to understand if one doesn't need to know the exact behavior of QGraphicsGridLayout (with which I also had a few problems with regards to row/column spacing IIRC).

> And I think the column setting is unnecessary... no? :)

I'm afraid I'm not sure what you are referring to.
Do you mean the ability to set the maximum number of columns in vertical formfactors?

> Anyway, as the freeze is near, I added an entry to the feature plan about this...

Ah, great. Thanks a lot!

Best regards,
Ingo


- Ingomar


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3786/#review5224
-----------------------------------------------------------


On 2010-04-23 19:08:34, Ingomar Wesp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3786/
> -----------------------------------------------------------
> 
> (Updated 2010-04-23 19:08:34)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> This is my proposed patch for the refactored quicklaunch applet as discussed in review request http://reviewboard.kde.org/r/3358/ and on the ML a while ago. Sorry that it took so long...
> 
> Summary of changes:
> - Refactored the quicklaunch applet, so that the applet,
>   icon grid widget and icon grid layout are split into
>   separate classes all living in a newly created namespace.
> 
> - Improved drag & drop behaviour (it is not possible to drop items
>   in the popup dialog) and drag & drop markers.
> 
> - Icons are now moved to/from the dialog explicitly instead of
>   asking the user to specify the number of the icons that are
>   shown in the primary area.
> 
> - Icon size is now determined automatically based on the
>   available space, hard-coded minimum and maximum bounds and
>   the number of rows (or columns) set by the user. This is done
>   in a custom layout that is no longer based on
>   QGraphicsGridLayout.
> 
> - When all icons are removed from an icon area, a placeholder
>   icon is displayed.
> 
> As this patch changes the configuration keys used, it also incorporates code for migrating older config keys.
> 
> Unfortunately, using svn diff with files that have been "svn move"d appears to yield broken diffs, so the patch here does not reflect the history for files that are based on renamed files (quicklaunch.*, quicklaunchicon.*), but I'll make sure that the history is preserved when committing (if this gets a "ship it", that is).
> 
> Please give it a spin and tell me what you think.
> 
> 
> This addresses bugs 206382, 206912, 214463, 225011, and 233914.
>     https://bugs.kde.org/show_bug.cgi?id=206382
>     https://bugs.kde.org/show_bug.cgi?id=206912
>     https://bugs.kde.org/show_bug.cgi?id=214463
>     https://bugs.kde.org/show_bug.cgi?id=225011
>     https://bugs.kde.org/show_bug.cgi?id=233914
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/CMakeLists.txt 1117710 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/QuicklaunchLayout.h 1117710 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/QuicklaunchLayout.cpp 1117710 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongrid.h PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongrid.cpp PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongridlayout.h PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongridlayout.cpp PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunch.h PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunch.cpp PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchApplet.h 1117710 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchApplet.cpp 1117710 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchConfig.ui 1117710 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchIcon.h 1117710 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchIcon.cpp 1117710 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchicon.h PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchicon.cpp PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/3786/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ingomar
> 
>