[GRASS-dev] [GRASS-SVN] r64921 - in grass/branches/releasebranch_7_0/gui/wxpython: gui_core lmgr

[Vaclav Petras]

Hi,

I'm sorry to criticize new features but I'm afraid it is necessary.

On Wed, Mar 25, 2015 at 4:26 PM, <svn_grass at osgeo.org> wrote:
>
> Author: martinl
> Date: 2015-03-25 13:26:02 -0700 (Wed, 25 Mar 2015)
> New Revision: 64921
>
> Modified:
>    grass/branches/releasebranch_7_0/gui/wxpython/gui_core/forms.py
>    grass/branches/releasebranch_7_0/gui/wxpython/lmgr/giface.py
> Log:
> wxGUI: gselect.Select() - show grouped maps from map display
>        LayerList - add len()
>        (merge r64855:6 from trunk)

First, this is a feature, not a bug fix. Do we backport features to 70
branch?

In we even backport features, is 11 days in between the original commit and
backport enough? It is quite a few days but I suppose that a lot of people
who were using trunk are now using release branch or even the stable
release, so the number of testers is lower than it was before 7.0.0 release.

> Modified: grass/branches/releasebranch_7_0/gui/wxpython/gui_core/forms.py
>                                  if maps_param and orig_elem == 'stds':
>                                      element_dict = {'raster': 'strds',
'vector': 'stvds', 'raster_3d': 'str3ds'}
>                                      elem =
element_dict[type_param.get('default')]
> -
> -                        if self._giface and hasattr(self._giface,
"_model"):
> -                            extraItems = {_('Graphical Modeler') :
self._giface.GetLayerList(p.get('prompt'))}
> -                        else:
> -                            extraItems = None
> +
> +                        extraItems = None
> +                        if self._giface:
> +                            if hasattr(self._giface, "_model"):
> +                                extraItems = {_('Graphical Modeler') :
self._giface.GetLayerList(p.get('prompt'))}
> +                            else:
> +                                layers = self._giface.GetLayerList()
> +                                if len(layers) > 0:
> +                                    mapList = []
> +                                    extraItems = {_('Map Display') :
mapList}
> +                                    for layer in layers:
> +                                        if layer.type != p.get('prompt'):
> +                                            continue
> +                                        mapList.append(str(layer))
>                          selection = gselect.Select(parent = which_panel,
id = wx.ID_ANY,
>                                                     size =
globalvar.DIALOG_GSELECT_SIZE,
>                                                     type = elem, multiple
= multiple, nmaps = len(p.get('key_desc', [])),

Also, this code is not well designed and although I might agree with
committing the code to trunk, I don't think that this should be backported
when it is not a bugfix.

There are two basic problems with the code. First, it does the same think
using giface but in two completely different ways. giface is here to avoid
these object specific conditions. This means that either the new code is
wrong or that the giface is not perfect. If later is true, then giface
should be fixed, not workarounded.

Second, how do you envision that this will work for different cases, e.g.
in g.gui.iclass? Will there be another condition with _("Classification
Tool")? The form.py should not depend on particular tools/components in GUI
nor it should contain the code which is specific to these tools and thus
this code should be part of these tools. Use of hasattr should always raise
a red flag. The way out in this case is probably better/extended giface
and/or parameter for the form dialog which allows to add or customize
source of additional items and its label.

I suggest to revert r64921 and significantly redo r64855.

Vaclav

https://trac.osgeo.org/grass/changeset/64855
https://trac.osgeo.org/grass/changeset/64856
https://trac.osgeo.org/grass/changeset/64921
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/grass-dev/attachments/20150403/afe89c6f/attachment.html>