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/
---

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 instruc

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

[Lukas Appelhans]

---
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 reco

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

[Lukas Appelhans]

---
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 reco

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

[Ingomar Wesp]

Hi!

Thanks for your ongoing support. I really appreciate it.

Lukas Appelhans wrote:
> 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 :))...

Are you referring to , maybe?

If you are, I pretty much agree with what FiNeX suggests (in fact, it 
would have even been easier to implement than the behavior that is in the
patch ;).

IMHO, an even better way to do it would be to mimic the behavior (and 
configuration options) of the task bar in that regard. That would allow the 
user to explicitly set the maximum number of rows (or columns in vertical form 
factors). It would also be quite flexible for power users who can force this 
number, should they not agree with the default height limit at which a new row 
is generated. Also, it would improve consistency across the various widgets.

If no one objects, I'll gladly implement that (but I'll finish this patch 
first).

> 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...

Whoops. This is one of the (probably many) artifacts left over from 
experimenting with different settings. I'll try to get rid of this an other 
nasty things before I submit the next diff. 

BTW (and sorry if this qustion is stupid, but I haven't dwelved far enough 
into the depths of Plasma): Is there any way to get a Plasma::Dialog to 
automatically resize to the preferredSize of its graphicsWidget upon change? 
Because that would be really helpful.

Alright. So I'll apply the following changes:

1.) Replace the configuration setting "visible icons" with something like ...
"enable dialog" (but that sounds a bit too technical), maybe ... "show 
more icons".

2.) Apply the functional changes required for 1).

3.) Add some graceful way of handling the empty grid situation.

4.) Polish the drop marker so it doesn't hurt to look at it.

If there are any objections, please don't hesitate to write a quick reply.

Ah, yes, one more question: Should I append the final patch to the existing 
review request or should I discard this one and create a new one when the time 
comes?

Best regards,
Ingo
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

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

[Lukas Appelhans]

Am Mittwoch 24 März 2010 17:47:07 schrieb Ingomar Wesp:
> Hi!
> 
> Thanks for your ongoing support. I really appreciate it.
> 
> Lukas Appelhans wrote:
> > 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 :))...
> 
> Are you referring to , maybe?
> 
> If you are, I pretty much agree with what FiNeX suggests (in fact, it
> would have even been easier to implement than the behavior that is in the
> patch ;).
> 
> IMHO, an even better way to do it would be to mimic the behavior (and
> configuration options) of the task bar in that regard. That would allow the
> user to explicitly set the maximum number of rows (or columns in vertical
> form factors). It would also be quite flexible for power users who can
> force this number, should they not agree with the default height limit at
> which a new row is generated. Also, it would improve consistency across
> the various widgets.
> 
> If no one objects, I'll gladly implement that (but I'll finish this patch
> first).
Sure, at the moment we also have the option to force a specific size but not 
having it in multiple rows (nookie requested that once I think)... we can 
abandon that though, I doubt anyone actually uses it :) Right? (open question 
to the list)
> 
> > 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...
> 
> Whoops. This is one of the (probably many) artifacts left over from
> experimenting with different settings. I'll try to get rid of this an other
> nasty things before I submit the next diff.
> 
> BTW (and sorry if this qustion is stupid, but I haven't dwelved far enough
> into the depths of Plasma): Is there any way to get a Plasma::Dialog to
> automatically resize to the preferredSize of its graphicsWidget upon
> change? Because that would be really helpful.
Mmh, I doubt this, but no real clue...
> 
> Alright. So I'll apply the following changes:
> 
> 1.) Replace the configuration setting "visible icons" with something like
> ... "enable dialog" (but that sounds a bit too technical), maybe ... "show
> more icons".
Yup, but then we don't really show more icons when checking this preference...

Mmh... "Enable additional popup", "Show popup for additional icons" etc... I 
have no clue :/ Anyone?
> 
> 2.) Apply the functional changes required for 1).
> 
> 3.) Add some graceful way of handling the empty grid situation.
> 
> 4.) Polish the drop marker so it doesn't hurt to look at it.
Great! :)
> 
> If there are any objections, please don't hesitate to write a quick reply.
> 
> Ah, yes, one more question: Should I append the final patch to the existing
> review request or should I discard this one and create a new one when the
> time comes?
Just append it I think...

Thanks,

Lukas
> 
> Best regards,
> Ingo
> ___
> Plasma-devel mailing list
> Plasma-devel@kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: 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-01 20:01:41.294461)


Review request for Plasma.


Changes
---

Changes as discussed on plasma-devel:

* Improved the appearance of the drop marker.
* Added a placeholder that is displayed when an icon area is empty.
* Replaced the configuration setting "visible icons" with something like 
  "enable popup" (suggestions for better wording welcome ;)

Is there anything important I should take into consideration when introducing
new user visible strings?


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 lis

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

[Lukas Appelhans]

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


All in all it looks good to me... I haven't tested it, but I found a pair of 
issues...

Thanks,

Lukas


/trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchicongrid.cpp


Why two layouts and why not as member-vars?



/trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchicongrid.cpp


Why this? We have this in the else-statement as well... :)



/trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchicongrid.cpp


If itemsChanged is false, then we don't even need to care about the stuff 
above... we just need to set the preferred size...


- Lukas


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 posi

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

[Ingomar Wesp]

> 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 a

Re: 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 s

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

[Lukas Appelhans]

Am Freitag 02 April 2010 20:49:48 schrieben Sie:
> You wrote:
> > > > 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.
> > 
> > Uhm well I'm worried...
> 
> So am I. The current behavior relies too much on the interaction of the two
> layouts and the size hints provided by the IconWidgets and is therefor too
> hard to grasp by just looking at the code.
> 
> > The icons are not center-aligned?
> 
> They are not currently, but that would be easy to change.
> 
> Please note that we are talking about horizontal alignment in horizontal
> panels / vertical alignment in vertical panels. The icon alignment forced
> by the spacer will only have an effect when the applet is given more than
> it's preferred width (horizontal panel) or height (vertical panels /
> desktop).
> 
> For this case it felt more natural to me to have the icons left (or top)
> aligned rather than have them centered. If this behavior is not desired,
> changing it would be extremely easy.
Mmh... I don't know what the others think about this, but for me centered was 
more natural... opinions from the list? :)

And I noticed that multiple row support is gone at the moment it seems :/

All in all it looks very nice... I need to think about the icon-size-
configurations still... :/

Thanks,

Lukas
> 
> So long,
> Ingomar

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

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

[Ingomar Wesp]

Sorry for the form of this reply (I'm writing this from my phone)...

! And I noticed that multiple row support is gone at the moment it seems :/

Multiple row support should work (unless you ran into a bug), but new rows are 
created only when there is enough space to fit the user-configurable icon size 
for each row. For an icon size of 32px, this means that the height of the panel 
needs to be at  least 64px for two rows to be shown.

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

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

[Ingomar Wesp]

You wrote:
>>> ! And I noticed that multiple row support is gone at the moment it seems
>>> :/
>> 
>> Multiple row support should work (unless you ran into a bug), but new
>> rows are created only when there is enough space to fit the
>> user-configurable icon size for each row. For an icon size of 32px, this
>> means that the height of the panel needs to be at  least 64px for two
>> rows to be shown.
> 
> Yes, well I was testing in the plasmoidviewer... maybe that was the
> problem...

Ok, maybe I could have done a better job at describing the way the layouting 
works in the patch right now:

When in a vertical panel or on the desktop (formFactor() != horizontal), the 
grid tries to create as many columns as possible and will only wrap to 
multiple rows if the horizontal space is not enough to hold all icons at their 
preferred size (AND there is enough vertical space to accomodate multiple rows 
at the icon size set by the user).

When placed on a horizontal panel, the grid tries to maximize the number of 
rows instead.

The reasoning is that you usually want the applet to make use of the vertical 
space on horizontal panels (multiple rows) or horizontal space on vertical 
panels (multiple columns), but you don't want it to create multiple rows / 
columns if this would mean that the icons are displayed smaller than you 
configured them to be.

The fact that this discussion exists is probably proof enough that we need to 
change the way creation of columns / rows are configured by the user.

In my opinion we should follow the example set by the task bar configuration, 
where the user basically sets the maximum number of rows (on horizontal 
panels) / the maximum number of columns (on vertical panels) with the option 
to force them if he doesn't like the default size at which wrapping takes 
place.

Max rows / max columns would be the same setting, just with a different label 
depending on the formFactor() of the applet. 

This suggestion would remove the need (and the ability) to set the icon size 
altogether. We would have to define some default row height at which wrapping 
takes place, but the user would be able to override it by setting "force 
number of rows"...

Please tell me what you think.

So long,
Ingomar
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

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

[Lukas Appelhans]

Am Montag 05 April 2010 17:00:29 schrieb Ingomar Wesp:
> You wrote:
> >>> ! And I noticed that multiple row support is gone at the moment it
> >>> seems
> >>> 
> >>> :/
> >> 
> >> Multiple row support should work (unless you ran into a bug), but new
> >> rows are created only when there is enough space to fit the
> >> user-configurable icon size for each row. For an icon size of 32px, this
> >> means that the height of the panel needs to be at  least 64px for two
> >> rows to be shown.
> > 
> > Yes, well I was testing in the plasmoidviewer... maybe that was the
> > problem...
> 
> Ok, maybe I could have done a better job at describing the way the
> layouting works in the patch right now:
> 
> When in a vertical panel or on the desktop (formFactor() != horizontal),
> the grid tries to create as many columns as possible and will only wrap to
> multiple rows if the horizontal space is not enough to hold all icons at
> their preferred size (AND there is enough vertical space to accomodate
> multiple rows at the icon size set by the user).
> 
> When placed on a horizontal panel, the grid tries to maximize the number of
> rows instead.
> 
> The reasoning is that you usually want the applet to make use of the
> vertical space on horizontal panels (multiple rows) or horizontal space on
> vertical panels (multiple columns), but you don't want it to create
> multiple rows / columns if this would mean that the icons are displayed
> smaller than you configured them to be.
> 
> The fact that this discussion exists is probably proof enough that we need
> to change the way creation of columns / rows are configured by the user.
> 
> In my opinion we should follow the example set by the task bar
> configuration, where the user basically sets the maximum number of rows
> (on horizontal panels) / the maximum number of columns (on vertical
> panels) with the option to force them if he doesn't like the default size
> at which wrapping takes place.
> 
> Max rows / max columns would be the same setting, just with a different
> label depending on the formFactor() of the applet.
> 
> This suggestion would remove the need (and the ability) to set the icon
> size altogether. We would have to define some default row height at which
> wrapping takes place, but the user would be able to override it by setting
> "force number of rows"...
> 
> Please tell me what you think.
Well go for it I think :) It's good to have some "standard" behaviour for 
certain things which we should follow at least in the applets which are 
shipped by default...

Thanks,

Lukas
> 
> So long,
> Ingomar

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

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

[Ingomar Wesp]

Lukas Appelhans wrote:
> Am Montag 05 April 2010 17:00:29 schrieb Ingomar Wesp:

>> This suggestion would remove the need (and the ability) to set the icon
>> size altogether. We would have to define some default row height at which
>> wrapping takes place, but the user would be able to override it by
>> setting "force number of rows"...
>> 
>> Please tell me what you think.
> 
> Well go for it I think :) It's good to have some "standard" behaviour for
> certain things which we should follow at least in the applets which are
> shipped by default...

Thanks for the encouragement. Then I'll go for it :)

Would you recommend that I commit the submitted patch and implement the 
changes on top of that or should I rather create one mega-patch that includes 
all the changes we discussed so far?

So long,
Ingomar

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel