<div dir="ltr"><div>Hi,<br><br></div><div>I'm sorry to criticize new features but I'm afraid it is necessary.<br></div><div><br>On Wed, Mar 25, 2015 at 4:26 PM, <<a href="mailto:svn_grass@osgeo.org">svn_grass@osgeo.org</a>> wrote:<br>><br>> Author: martinl<br>> Date: 2015-03-25 13:26:02 -0700 (Wed, 25 Mar 2015)<br>> New Revision: 64921<br>><br>> Modified:<br>>    grass/branches/releasebranch_7_0/gui/wxpython/gui_core/forms.py<br>>    grass/branches/releasebranch_7_0/gui/wxpython/lmgr/giface.py<br>> Log:<br>> wxGUI: gselect.Select() - show grouped maps from map display<br>>        LayerList - add len()<br>>        (merge r64855:6 from trunk)<br><br></div><div>First, this is a feature, not a bug fix. Do we backport features to 70 branch?<br></div><div><br></div>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.<br><div><br>> Modified: grass/branches/releasebranch_7_0/gui/wxpython/gui_core/forms.py<br>>                                  if maps_param and orig_elem == 'stds':<br>>                                      element_dict = {'raster': 'strds', 'vector': 'stvds', 'raster_3d': 'str3ds'}<br>>                                      elem = element_dict[type_param.get('default')]<br>> -<br>> -                        if self._giface and hasattr(self._giface, "_model"):<br>> -                            extraItems = {_('Graphical Modeler') : self._giface.GetLayerList(p.get('prompt'))}<br>> -                        else:<br>> -                            extraItems = None<br>> +<br>> +                        extraItems = None<br>> +                        if self._giface:<br>> +                            if hasattr(self._giface, "_model"):<br>> +                                extraItems = {_('Graphical Modeler') : self._giface.GetLayerList(p.get('prompt'))}<br>> +                            else:<br>> +                                layers = self._giface.GetLayerList()<br>> +                                if len(layers) > 0:<br>> +                                    mapList = []<br>> +                                    extraItems = {_('Map Display') : mapList}<br>> +                                    for layer in layers:<br>> +                                        if layer.type != p.get('prompt'):<br>> +                                            continue<br>> +                                        mapList.append(str(layer))<br>>                          selection = gselect.Select(parent = which_panel, id = wx.ID_ANY,<br>>                                                     size = globalvar.DIALOG_GSELECT_SIZE,<br>>                                                     type = elem, multiple = multiple, nmaps = len(p.get('key_desc', [])),<br><br></div><div>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.<br></div><div><br></div><div>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.<br><br></div><div>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.<br></div><div><br></div><div>I suggest to revert r64921 and significantly redo r64855.</div><div><br></div><div>Vaclav<br></div><div><br><a href="https://trac.osgeo.org/grass/changeset/64855">https://trac.osgeo.org/grass/changeset/64855</a><br><a href="https://trac.osgeo.org/grass/changeset/64856">https://trac.osgeo.org/grass/changeset/64856</a><br><a href="https://trac.osgeo.org/grass/changeset/64921">https://trac.osgeo.org/grass/changeset/64921</a><br></div></div>