----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/3358/#review4632 -----------------------------------------------------------
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 > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel