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

[Ingomar Wesp]

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

(Updated 2010-04-02 19:05:06.237279)


Review request for Plasma.


Changes
-------

As discussed with Lukas:

* Make the two layouts in QuicklaunchIconGrid available as member vars.
* Remove unnecessary call to QuicklaunchIcon::setPreferredSize(...).

Horizontal alignment of icons when there is excess horizontal space (in horizontal panels) or vertical alignment when there is excess vertical space (in vertical panels or the desktop) remains unchanged for now, but is easy to change if requested.


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 (updated)
-----

  /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/CMakeLists.txt 1109698 
  /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/QuicklaunchLayout.h 1110330 
  /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/QuicklaunchLayout.cpp 1110330 
  /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