Review Request: [Quicklaunch] Refactoring of the Quicklaunch applet

[Lukas Appelhans]

Am Montag 26 April 2010 13:11:43 schrieben Sie:
> > 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?
No, in horizontal formfactors there's an option to force the number of 
columns, it's just a checkbox...

Lukas
> 
> > 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/CMakeLi
> >   sts.txt 1117710
> >   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/Quickl
> >   aunchLayout.h 1117710
> >   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/Quickl
> >   aunchLayout.cpp 1117710
> >   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongr
> >   id.h PRE-CREATION
> >   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongr
> >   id.cpp PRE-CREATION
> >   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongr
> >   idlayout.h PRE-CREATION
> >   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongr
> >   idlayout.cpp PRE-CREATION
> >   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quickl
> >   aunch.h PRE-CREATION
> >   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quickl
> >   aunch.cpp PRE-CREATION
> >   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quickl
> >   aunchApplet.h 1117710
> >   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quickl
> >   aunchApplet.cpp 1117710
> >   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quickl
> >   aunchConfig.ui 1117710
> >   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quickl
> >   aunchIcon.h 1117710
> >   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quickl
> >   aunchIcon.cpp 1117710
> >   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quickl
> >   aunchicon.h PRE-CREATION
> >   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quickl
> >   aunchicon.cpp PRE-CREATION
> > 
> > Diff: http://reviewboard.kde.org/r/3786/diff
> > 
> > 
> > Testing
> > -------
> > 
> > 
> > Thanks,
> > 
> > Ingomar