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

Lukas Appelhans l.appelhans at gmx.de
Tue Mar 23 20:48:47 CET 2010 

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


Hey!

First of all, I agree with pretty much all the points you noted! :) I also have or better had some ideas about how to make the configuration of icon-size/icon-rows cleaner and easier to use (the original idea was from FiNeX, I have to talk to him again :))...

Anyway, I will give the patch a final review when it's finished, but one thing I noticed from a short look was that you use quite a bunch of magic numbers in your code, e.g. "dialogSize.setWidth(dialogSize.width() + 14);" whereas you should rather move the "14" into a const int NUMBER...

Thanks for your work... :)

Lukas

- Lukas


On 2010-03-23 13:37:38, Ingomar Wesp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3358/
> -----------------------------------------------------------
> 
> (Updated 2010-03-23 13:37:38)
> 
> 
> 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 bug 206382.
>     https://bugs.kde.org/show_bug.cgi?id=206382
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/CMakeLists.txt 1103058 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/QuicklaunchLayout.h 1106634 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/QuicklaunchLayout.cpp 1106634 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchApplet.h 1103058 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchApplet.cpp 1103058 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchIcon.h 1023511 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchIcon.cpp 1023511 
>   /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