Review Request: [1/3] Refactor EnvironmentSelectionWidget.

[Milian Wolff]

> On Jan. 6, 2013, 11:10 a.m., Milian Wolff wrote:
> > I missed the "why" for this patch set - can you elaborate please why this is required?
> 
> Ivan Shapovalov wrote:
>     Reasons were following:
>     
>     1. Every user of this class had to write the same code:
>         EnvironmentGroupList env( KGlobal::config() );
>         environment.addItems( env.groups() );
>     
>     2. Nobody seemed to use EnvironmentGroupList::defaultGroup(), relying on "default" group being default - so, effectively, "Set group as default" in configuration KCM did nothing.
>     
>     3. If KConfig value associated with the widget gets empty somehow, widget displays an empty line - which is not user-friendly.
>     
>     4. Summarizing, EnvironmentSelectionWidget is a wrapper class that does nothing (well, apart from saving item text instead of item index to the KConfig).
>     
>     Are these reasons ok?
>
> 
> Andreas Pakulat wrote:
>     I don't have a strong opinion on any of this, except the storage. That should stay as it is and store the name of the environment group and not the index in the combobox. For the simple reason that the list is dynamic and there's no guarantee of order for the groups. Otherwise you'd have to store the index of each group along with the group and then store the current index which means adding unecessary bloat to the config files.
> 
> Ivan Shapovalov wrote:
>     Of course, I did not change the storage. It stores the group name.

The reasons sound good enough to me (please make sure to include them in your final commit).

Still, I do think you should refactor the selection widget to not inherit from KComboBox any longer.


- Milian


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


On Jan. 5, 2013, 9:22 p.m., Ivan Shapovalov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108212/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2013, 9:22 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> Feature-wise this gets rid of dependency on default KConfigXT value being "default" and allows explicitly specifying to use a default environment group whatever it will be.
> 
> Now an empty value shall be used by default, which has meaning of "use whatever is set by default at runtime".
> Also, a special "Use default" line is now always added to the combo-box which maps to the empty config value, so user won't see empty entries in the combo-box.
> 
> 
> Diffs
> -----
> 
>   plugins/execute/nativeappconfig.cpp 481d56c 
>   plugins/executescript/scriptappconfig.cpp b8f0729 
>   util/CMakeLists.txt 29de126 
>   util/environmentconfigurebutton.cpp 8303c70 
>   util/environmentselectionmodel.h PRE-CREATION 
>   util/environmentselectionmodel.cpp PRE-CREATION 
>   util/environmentselectionwidget.h 0a69afc 
>   util/environmentselectionwidget.cpp 63fdebc 
> 
> Diff: http://git.reviewboard.kde.org/r/108212/diff/
> 
> 
> Testing
> -------
> 
> Manual testing.
> 
> 
> Thanks,
> 
> Ivan Shapovalov
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20130106/e31553a8/attachment.html>