Review Request: [Quicklaunch] Refactoring, layout fixes and a drag & drop marker.

Ingomar Wesp ingomar at wesp.name
Fri Apr 2 00:05:36 CEST 2010 


> On 2010-04-01 21:18:42, Lukas Appelhans wrote:
> > All in all it looks good to me... I haven't tested it, but I found a pair of issues...
> > 
> > Thanks,
> > 
> > Lukas

Thanks for having a look :)


> On 2010-04-01 21:18:42, Lukas Appelhans wrote:
> > /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchicongrid.cpp, line 205
> > <http://reviewboard.kde.org/r/3358/diff/2/?file=22420#file22420line205>
> >
> >     Why this? We have this in the else-statement as well... :)

You're right, this is unnecessary. I'll remove it shortly.


> On 2010-04-01 21:18:42, Lukas Appelhans wrote:
> > /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchicongrid.cpp, line 135
> > <http://reviewboard.kde.org/r/3358/diff/2/?file=22420#file22420line135>
> >
> >     Why two layouts and why not as member-vars?

> Why two layouts

I'm currently using the outer linear layout because I need a spacer (stretch) that pushes the icons to the top (for horizontal panel/desktop) or to the left (in vertical panels) if there is more space available than needed.

This also prevents the inner grid layout from resizing the individual icons beyond their preferred size because of excess space.

> why not as member-vars

I didn't see the need in saving the pointers in member vars as they are accessible through layout() and layout->itemAt(0). I do agree, however, that the latter is pretty ugly (as is the casting required to use them).

In the long run, I think it would be preferrable to put all the layouting code in a custom QGraphicsLayout subclass that is not a subclass of QGraphicsGridLayout (as this would imply that we constantly have to add/remove items in order to get the wrapping right). Something like a flow layout with a fixed cell size, maybe.


> On 2010-04-01 21:18:42, Lukas Appelhans wrote:
> > /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchicongrid.cpp, line 486
> > <http://reviewboard.kde.org/r/3358/diff/2/?file=22420#file22420line486>
> >
> >     If itemsChanged is false, then we don't even need to care about the stuff above... we just need to set the preferred size...

Well, actually we do have to care, as we need to check whether the current row/column wrapping is still valid after a resize, change of orientation or a change to the icon size hint.


- Ingomar


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


On 2010-04-01 20:01:41, Ingomar Wesp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3358/
> -----------------------------------------------------------
> 
> (Updated 2010-04-01 20:01:41)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> Ok, this is a biggie (and still a work in progress), so there are certainly a 
> bunch of issues. I just wanted to let you know that I've started to refactor 
> the quicklaunch applet hoping to make it a bit easier to address existing 
> issues and to add new features. 
> 
> As this turned out to be more of an undertaking than I originally thought and 
> since it is now now in a state where it's not completely broken ;), I thought 
> I'd collect some feedback on the patch thus far. Especially since I 
> accumulated a number of questions regarding decisions I can't  
> 
> Without further ado, these are the main points of the patch:
> 
> - Moved icon layout, memory management and drag & drop handling into a new 
>   widget (QuicklaunchIconGrid), so not everything has to be handled by the 
>   QuicklaunchApplet class. However, since the existing logic requires that
>   icons are pushed to / pulled from the dialog depending on the total number
>   of icons, I couldn't get rid of some icon management code in the applet
>   quite yet (more about that later).
> 
> - Changed the way the icon size is used in order to make it work better in 
>   size constrained conditions. The user configurable icon size is now used as
>   a hint that restricts the creation of new columns (in vertical form factors)
>   or rows (in horzional form factors) when this would mean that the configured
>   size would be undershot. Icons are now allowed to scale down below the 
>   set icon size if there is not enough space available. 
> 
> - Added preliminary display of drop markers (as of now: in the form of ugly
>   gray squares) when dragging icons around.
> 
> - Added drag & drop support to the dialog.
> 
> Known regressions / still missing:
> 
> - Sorting of icons is not yet re-implemented.
> 
> - The code is still quite hacky at some points (but I'll wait with cleanup 
>   until I know about the outcome of the discussion about the questions below).
> 
> - The primary icon area is not repopulated once all icons are removed.
> 
> Although I tried to preserve the current functionality during the rewrite, I 
> ran into a few design decisions that I thought might deserve some discussion:
> 
> ----
> 1.) The dialog (and it's configuration by the number visible icons)
> 
> Especially since drag & drop to the dialog works now (or at least it 
> should ;), I think it feels odd (from a user perspective) that the separation
> between icons in the primary area and icons in the dialog needs to be manually
> configured by setting the number of icons that should be displayed by the main 
> area.
> 
> Consider the following use case:
> 
> Joe uses the quicklaunch applet for starting some apps he uses regularly 
> (which he wants to be reachable from the primary area) as well as for apps he
> uses sometimes, but not too often (which he wants to be in the dialog). Lets 
> say, his favourite apps are called FooApp and BarApp. In order to configure 
> what he wants, he needs to
> 
> - add his favourite 2 apps to the quicklaunch applet
> - order the icons so that they are at indices 0 and 1 respectively.
> - open the config dialog and set the number of visible icons to 2
> 
> After a few days where he also added a number of other programs to the dialog, 
> he discovers a new program he really likes: BazApp (obviously). In order
> to add it to the main area, he would have to:
> 
> - open the config dialog and set the number of visible icons to 3.
> - notice that the first program that previously was on the dialog (OtherApp) 
>   popped into the main area.
> - drop BarApp in the main area at a position where it pushes OtherApp back 
>   into the dialog.
> 
> Had Joe forgot about how the applet works, he might have tried to drag BarApp 
> to the main area before reconfiguring the applet, which would have caused it 
> to be pushed into the (possibly hidden) dialog immediately. 
> 
> As a first step to a solution, I would suggest changing the behaviour so that 
> the user simply chooses whether to show a dialog or not. If the dialog is
> enabled, items can be freely dragged to / from the dialog or the main area (or 
> added / removed using the context menu). This would require to display some 
> default content when the icon areas are empty (but that would probably be a 
> good idea anyway - see 2).
> 
> This change would not even require a change to the applet's config as the 
> icons could still be serialized into a single list of URLs separated by the 
> count of URLs in the primary area.
> 
> 2.) Behaviour when an icon area is empty
> 
> Until now, the icon area got repopulated with the default icons after all 
> icons have been removed manually. This might be annoying for users who don't
> know about that and just want to clear the area in order to set their own 
> icons.
> 
> Maybe we should simply display an icon (say, the quicklaunch logo in disabled 
> state) that shows some instructional text in its tooltip (something like "Drag 
> programs here", just better ;) when there are no icons to display.
> 
> 3.) The "Sort Alphabetically" actions
> 
> I hope I'm not stepping on anyone's toes, but I'm not 100% sure about the need 
> for this particular feature, since the number of icons that are handled 
> reasonably by the quicklaunch applet is rather small and adding/removing of 
> icons needs to be done by hand anyways.
> 
> ---
> 
> Well, that's it for now. Please let me know what you think.
> 
> Oh, and please excuse that I won't be able to answer immediately (as I need to 
> go on a short trip in a few minutes).
> 
> Best regards,
> Ingo
> 
> 
> This addresses bugs 206382 and 214463.
>     https://bugs.kde.org/show_bug.cgi?id=206382
>     https://bugs.kde.org/show_bug.cgi?id=214463
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/CMakeLists.txt 1109698 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/QuicklaunchLayout.h 1110070 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/QuicklaunchLayout.cpp 1110070 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchApplet.h 1109698 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchApplet.cpp 1109698 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchConfig.ui 1109698 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchIcon.h 1109698 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchIcon.cpp 1109698 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchicongrid.h PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchicongrid.cpp PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/3358/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ingomar
> 
>

More information about the Plasma-devel mailing list