Re: Review Request: [Quicklaunch] Refactoring of the Quicklaunch applet
Lukas Appelhans wrote: It's just as easy as changing the .desktop file and putting your name into it instead of mine :) Oh, well, I should manage to do that ;) And of course taking care of it... :) I'll treat it gently ;) (You'll also need an SVN-Account) I've already got one, so that's no problem. Maybe I should also try to get myself registered as default assignee for widget-quicklaunch in Bugzilla... Can this be done by filing a sysadmin request? Best regards and thanks for mentoring me during my first steps in KDE dev land. Ingo ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: [Quicklaunch] Refactoring of the Quicklaunch applet
Am Mittwoch 28 April 2010 18:10:54 schrieb Ingomar Wesp: Lukas Appelhans wrote: It's just as easy as changing the .desktop file and putting your name into it instead of mine :) Oh, well, I should manage to do that ;) And of course taking care of it... :) I'll treat it gently ;) (You'll also need an SVN-Account) I've already got one, so that's no problem. Maybe I should also try to get myself registered as default assignee for widget-quicklaunch in Bugzilla... Can this be done by filing a sysadmin request? I think so... :) Best regards and thanks for mentoring me during my first steps in KDE dev land. Hehe, thanks for taking some points off my todo list :) Lukas 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 of the Quicklaunch applet
Am Montag 26 April 2010 15:48:27 schrieb Ingomar Wesp: And I think the column setting is unnecessary... no? :) I'm afraid I'm not sure what you are referring to. Do you mean the ability to set the maximum number of columns in vertical formfactors? No, in horizontal formfactors there's an option to force the number of columns, it's just a checkbox... Ah, now I think I know what you mean ;) If I'm right, you are referring to the planar form factor (applet on desktop / in plasmoidviewer). If you would see Force column settings in the settings of the quicklaunch applet in a horizontal form factor (for example, in a horizontal panel), that would be a bug … Currently, the configuration settings are (actually, the underlying settings are the same, but the user visible strings are different): formFactor == Plasma::horizontal: Force row settings Max rows count else Force column settings Max column count But you are right that Force column settings is probably unnecessary in a planar form factor (at least I'm having trouble finding a use case for that). If you can confirm that I'm understanding you right, I'll remove the visibility for this setting in this case. Ahh true, it was force column settings as the applet was in plasmoidviewer... in panel it worked greatly (I had overlooked/wrong seen some stuff it seems... I just tested again =))... Can you make the configuration of the rows first and then have the force setting (aka swap the place of them), that'd make more sense to me as the user reads the options from top to bottom, not knowing that there are any row/column settings... :) Except that the patch is absuletely fine to go in... the applet now has a much better quality than before... :) Btw, as you pretty much rewrote the applet, may you also take over the maintainership? :) 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 of the Quicklaunch applet
Lukas Appelhans wrote: Can you make the configuration of the rows first and then have the force setting (aka swap the place of them), that'd make more sense to me as the user reads the options from top to bottom, not knowing that there are any row/column settings... :) That's a good idea, seems more plausible to me as well. I'll change that before committing. Except that the patch is absuletely fine to go in... the applet now has a much better quality than before... :) Thanks, I'm very happy to hear that. Btw, as you pretty much rewrote the applet, may you also take over the maintainership? :) I'd love to :) Being a bit of a newbie still, however, I don't know whom to contact / what to do in order to get this underway. So I'm afraid I'm going to need a few hints in this regards. Thanks for your continued support. Best regards, Ingo ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: [Quicklaunch] Refactoring of the Quicklaunch applet
Am Dienstag 27 April 2010 21:48:48 schrieb Ingomar Wesp: Lukas Appelhans wrote: Can you make the configuration of the rows first and then have the force setting (aka swap the place of them), that'd make more sense to me as the user reads the options from top to bottom, not knowing that there are any row/column settings... :) That's a good idea, seems more plausible to me as well. I'll change that before committing. Except that the patch is absuletely fine to go in... the applet now has a much better quality than before... :) Thanks, I'm very happy to hear that. Btw, as you pretty much rewrote the applet, may you also take over the maintainership? :) I'd love to :) Being a bit of a newbie still, however, I don't know whom to contact / what to do in order to get this underway. So I'm afraid I'm going to need a few hints in this regards. It's just as easy as changing the .desktop file and putting your name into it instead of mine :) And of course taking care of it... :) (You'll also need an SVN-Account then, but that's easy to request...) Lukas Thanks for your continued support. 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 of the Quicklaunch applet
On None, Lukas Appelhans wrote: Hey! Great work! I had a look over the code and tested it and it works greatly! 2 things: Why do we use a custom layout instead of a QGridLayout? And I think the column setting is unnecessary... no? :) Anyway, as the freeze is near, I added an entry to the feature plan about this... I would say +1 for committing after the 2 things have been discussed... Lukas Hi! Great work! I had a look over the code and tested it and it works greatly! Thanks, I'm happy to hear that. Why do we use a custom layout instead of a QGridLayout? 2 reasons, primarily. For one, I didn't want to have to completely repopulate the underlying QGraphicsGridLayout every time the icon wrapping changes (caused by resizes or changes to the applet's configuration). Secondly, I thought that the code is simpler to understand if one doesn't need to know the exact behavior of QGraphicsGridLayout (with which I also had a few problems with regards to row/column spacing IIRC). And I think the column setting is unnecessary... no? :) I'm afraid I'm not sure what you are referring to. Do you mean the ability to set the maximum number of columns in vertical formfactors? Anyway, as the freeze is near, I added an entry to the feature plan about this... Ah, great. Thanks a lot! Best regards, Ingo - Ingomar --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/3786/#review5224 --- On 2010-04-23 19:08:34, Ingomar Wesp wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/3786/ --- (Updated 2010-04-23 19:08:34) Review request for Plasma. Summary --- This is my proposed patch for the refactored quicklaunch applet as discussed in review request http://reviewboard.kde.org/r/3358/ and on the ML a while ago. Sorry that it took so long... Summary of changes: - Refactored the quicklaunch applet, so that the applet, icon grid widget and icon grid layout are split into separate classes all living in a newly created namespace. - Improved drag drop behaviour (it is not possible to drop items in the popup dialog) and drag drop markers. - Icons are now moved to/from the dialog explicitly instead of asking the user to specify the number of the icons that are shown in the primary area. - Icon size is now determined automatically based on the available space, hard-coded minimum and maximum bounds and the number of rows (or columns) set by the user. This is done in a custom layout that is no longer based on QGraphicsGridLayout. - When all icons are removed from an icon area, a placeholder icon is displayed. As this patch changes the configuration keys used, it also incorporates code for migrating older config keys. Unfortunately, using svn diff with files that have been svn moved appears to yield broken diffs, so the patch here does not reflect the history for files that are based on renamed files (quicklaunch.*, quicklaunchicon.*), but I'll make sure that the history is preserved when committing (if this gets a ship it, that is). Please give it a spin and tell me what you think. This addresses bugs 206382, 206912, 214463, 225011, and 233914. https://bugs.kde.org/show_bug.cgi?id=206382 https://bugs.kde.org/show_bug.cgi?id=206912 https://bugs.kde.org/show_bug.cgi?id=214463 https://bugs.kde.org/show_bug.cgi?id=225011 https://bugs.kde.org/show_bug.cgi?id=233914 Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/CMakeLists.txt 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/QuicklaunchLayout.h 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/QuicklaunchLayout.cpp 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongrid.h PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongrid.cpp PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongridlayout.h PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongridlayout.cpp PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunch.h PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunch.cpp PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchApplet.h 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchApplet.cpp 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchConfig.ui
Re: Review Request: [Quicklaunch] Refactoring of the Quicklaunch applet
Am Montag 26 April 2010 13:11:43 schrieben Sie: On None, Lukas Appelhans wrote: Hey! Great work! I had a look over the code and tested it and it works greatly! 2 things: Why do we use a custom layout instead of a QGridLayout? And I think the column setting is unnecessary... no? :) Anyway, as the freeze is near, I added an entry to the feature plan about this... I would say +1 for committing after the 2 things have been discussed... Lukas Hi! Great work! I had a look over the code and tested it and it works greatly! Thanks, I'm happy to hear that. Why do we use a custom layout instead of a QGridLayout? 2 reasons, primarily. For one, I didn't want to have to completely repopulate the underlying QGraphicsGridLayout every time the icon wrapping changes (caused by resizes or changes to the applet's configuration). Secondly, I thought that the code is simpler to understand if one doesn't need to know the exact behavior of QGraphicsGridLayout (with which I also had a few problems with regards to row/column spacing IIRC). And I think the column setting is unnecessary... no? :) I'm afraid I'm not sure what you are referring to. Do you mean the ability to set the maximum number of columns in vertical formfactors? No, in horizontal formfactors there's an option to force the number of columns, it's just a checkbox... Lukas Anyway, as the freeze is near, I added an entry to the feature plan about this... Ah, great. Thanks a lot! Best regards, Ingo - Ingomar --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/3786/#review5224 --- On 2010-04-23 19:08:34, Ingomar Wesp wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/3786/ --- (Updated 2010-04-23 19:08:34) Review request for Plasma. Summary --- This is my proposed patch for the refactored quicklaunch applet as discussed in review request http://reviewboard.kde.org/r/3358/ and on the ML a while ago. Sorry that it took so long... Summary of changes: - Refactored the quicklaunch applet, so that the applet, icon grid widget and icon grid layout are split into separate classes all living in a newly created namespace. - Improved drag drop behaviour (it is not possible to drop items in the popup dialog) and drag drop markers. - Icons are now moved to/from the dialog explicitly instead of asking the user to specify the number of the icons that are shown in the primary area. - Icon size is now determined automatically based on the available space, hard-coded minimum and maximum bounds and the number of rows (or columns) set by the user. This is done in a custom layout that is no longer based on QGraphicsGridLayout. - When all icons are removed from an icon area, a placeholder icon is displayed. As this patch changes the configuration keys used, it also incorporates code for migrating older config keys. Unfortunately, using svn diff with files that have been svn moved appears to yield broken diffs, so the patch here does not reflect the history for files that are based on renamed files (quicklaunch.*, quicklaunchicon.*), but I'll make sure that the history is preserved when committing (if this gets a ship it, that is). Please give it a spin and tell me what you think. This addresses bugs 206382, 206912, 214463, 225011, and 233914. https://bugs.kde.org/show_bug.cgi?id=206382 https://bugs.kde.org/show_bug.cgi?id=206912 https://bugs.kde.org/show_bug.cgi?id=214463 https://bugs.kde.org/show_bug.cgi?id=225011 https://bugs.kde.org/show_bug.cgi?id=233914 Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/CMakeLi sts.txt 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/Quickl aunchLayout.h 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/Quickl aunchLayout.cpp 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongr id.h PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongr id.cpp PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongr idlayout.h PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongr idlayout.cpp PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quickl aunch.h PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quickl aunch.cpp PRE-CREATION
Re: Review Request: [Quicklaunch] Refactoring of the Quicklaunch applet
And I think the column setting is unnecessary... no? :) I'm afraid I'm not sure what you are referring to. Do you mean the ability to set the maximum number of columns in vertical formfactors? No, in horizontal formfactors there's an option to force the number of columns, it's just a checkbox... Ah, now I think I know what you mean ;) If I'm right, you are referring to the planar form factor (applet on desktop / in plasmoidviewer). If you would see Force column settings in the settings of the quicklaunch applet in a horizontal form factor (for example, in a horizontal panel), that would be a bug … Currently, the configuration settings are (actually, the underlying settings are the same, but the user visible strings are different): formFactor == Plasma::horizontal: Force row settings Max rows count else Force column settings Max column count But you are right that Force column settings is probably unnecessary in a planar form factor (at least I'm having trouble finding a use case for that). If you can confirm that I'm understanding you right, I'll remove the visibility for this setting in this case. Best regards, Ingo ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: [Quicklaunch] Refactoring of the Quicklaunch applet
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/3786/#review5225 --- - Lukas On 2010-04-23 19:08:34, Ingomar Wesp wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/3786/ --- (Updated 2010-04-23 19:08:34) Review request for Plasma. Summary --- This is my proposed patch for the refactored quicklaunch applet as discussed in review request http://reviewboard.kde.org/r/3358/ and on the ML a while ago. Sorry that it took so long... Summary of changes: - Refactored the quicklaunch applet, so that the applet, icon grid widget and icon grid layout are split into separate classes all living in a newly created namespace. - Improved drag drop behaviour (it is not possible to drop items in the popup dialog) and drag drop markers. - Icons are now moved to/from the dialog explicitly instead of asking the user to specify the number of the icons that are shown in the primary area. - Icon size is now determined automatically based on the available space, hard-coded minimum and maximum bounds and the number of rows (or columns) set by the user. This is done in a custom layout that is no longer based on QGraphicsGridLayout. - When all icons are removed from an icon area, a placeholder icon is displayed. As this patch changes the configuration keys used, it also incorporates code for migrating older config keys. Unfortunately, using svn diff with files that have been svn moved appears to yield broken diffs, so the patch here does not reflect the history for files that are based on renamed files (quicklaunch.*, quicklaunchicon.*), but I'll make sure that the history is preserved when committing (if this gets a ship it, that is). Please give it a spin and tell me what you think. This addresses bugs 206382, 206912, 214463, 225011, and 233914. https://bugs.kde.org/show_bug.cgi?id=206382 https://bugs.kde.org/show_bug.cgi?id=206912 https://bugs.kde.org/show_bug.cgi?id=214463 https://bugs.kde.org/show_bug.cgi?id=225011 https://bugs.kde.org/show_bug.cgi?id=233914 Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/CMakeLists.txt 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/QuicklaunchLayout.h 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/QuicklaunchLayout.cpp 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongrid.h PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongrid.cpp PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongridlayout.h PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongridlayout.cpp PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunch.h PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunch.cpp PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchApplet.h 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchApplet.cpp 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchConfig.ui 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchIcon.h 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchIcon.cpp 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchicon.h PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunchicon.cpp PRE-CREATION Diff: http://reviewboard.kde.org/r/3786/diff Testing --- Thanks, Ingomar ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: [Quicklaunch] Refactoring of the Quicklaunch applet
Lukas Appelhans wrote: For keeping the history we usually use svn copy and then change the files :) svn move only works with svn paths, not with local paths... Please correct me if I'm wrong, but as far as I know svn move *does* work for working copy paths and yields the same result as svn copy a b followed by svn delete a. I will test review the patch soon :) Great, thanks! Have a nice day, Ingo ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: [Quicklaunch] Refactoring of the Quicklaunch applet
Am Freitag 23 April 2010 21:08:34 schrieb Ingomar Wesp: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/3786/ --- Review request for Plasma. Summary --- This is my proposed patch for the refactored quicklaunch applet as discussed in review request http://reviewboard.kde.org/r/3358/ and on the ML a while ago. Sorry that it took so long... Summary of changes: - Refactored the quicklaunch applet, so that the applet, icon grid widget and icon grid layout are split into separate classes all living in a newly created namespace. - Improved drag drop behaviour (it is not possible to drop items in the popup dialog) and drag drop markers. - Icons are now moved to/from the dialog explicitly instead of asking the user to specify the number of the icons that are shown in the primary area. - Icon size is now determined automatically based on the available space, hard-coded minimum and maximum bounds and the number of rows (or columns) set by the user. This is done in a custom layout that is no longer based on QGraphicsGridLayout. - When all icons are removed from an icon area, a placeholder icon is displayed. As this patch changes the configuration keys used, it also incorporates code for migrating older config keys. Unfortunately, using svn diff with files that have been svn moved appears to yield broken diffs, so the patch here does not reflect the history for files that are based on renamed files (quicklaunch.*, quicklaunchicon.*), but I'll make sure that the history is preserved when committing (if this gets a ship it, that is). For keeping the history we usually use svn copy and then change the files :) svn move only works with svn paths, not with local paths... I will test review the patch soon :) Thanks Lukas Please give it a spin and tell me what you think. This addresses bugs 206382, 206912, 214463, 225011, and 233914. https://bugs.kde.org/show_bug.cgi?id=206382 https://bugs.kde.org/show_bug.cgi?id=206912 https://bugs.kde.org/show_bug.cgi?id=214463 https://bugs.kde.org/show_bug.cgi?id=225011 https://bugs.kde.org/show_bug.cgi?id=233914 Diffs - /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/CMakeLists .txt 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/Quicklaunc hLayout.h 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/Quicklaunc hLayout.cpp 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongrid.h PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongrid.c pp PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongridla yout.h PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/icongridla yout.cpp PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunc h.h PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunc h.cpp PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunc hApplet.h 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunc hApplet.cpp 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunc hConfig.ui 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunc hIcon.h 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunc hIcon.cpp 1117710 /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunc hicon.h PRE-CREATION /trunk/KDE/kdebase/workspace/plasma/generic/applets/quicklaunch/quicklaunc hicon.cpp PRE-CREATION Diff: http://reviewboard.kde.org/r/3786/diff Testing --- Thanks, Ingomar ___ 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
