Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wxGUI/vdigit: clean zero-length vector map boundaries before start editing #911

Closed
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions gui/wxpython/vdigit/toolbars.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,23 @@
@author Martin Landa <landa.martin gmail.com>
@author Stepan Turek <stepan.turek seznam.cz> (handlers support)
"""

from ctypes import c_char_p, pointer

import wx

from grass import script as grass
from grass.lib.vector import GV_BOUNDARY, GV_LINE, GV_LINES, Map_info, \
Vect_close, Vect_get_line_type, Vect_get_num_lines, Vect_line_alive, \
Vect_line_length, Vect_new_cats_struct, Vect_new_line_struct, \
Vect_open_old, Vect_read_line
Comment on lines +18 to +26
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of ctypes and the generated ctypes interface to libraries is tricky already at the import level. Other places in GUI which do this are (or should be) very careful about import. It is best to completely avoid this in GUI and solve it in a separate module (in Python or C).

from grass.pydispatch.signal import Signal

from gui_core.toolbars import BaseToolbar, BaseIcons
from gui_core.dialogs import CreateNewVector, VectorDialog
from gui_core.wrap import PseudoDC, Menu
from vdigit.preferences import VDigitSettingsDialog
from vnet.vnet_utils import ParseMapStr
from core.debug import Debug
from core.settings import UserSettings
from core.gcmd import GError, RunCommand
Expand Down Expand Up @@ -858,6 +866,62 @@ def OnSelectMap(self, event):

event.Skip()

def _topoZeroLengthCheck(self, map_name):
"""Topology check for lines or boundaries of zero length

:param str map_name: full vector map name
Comment on lines +869 to +872
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code in this function should probably be a separate module. It should not be part of GUI because it is data processing. In general, we should be moving any processing and analytical code from GUI. The idea is to keep the GUI as small as possible while making all functionality available in GUI also available for scripting ideally in command line or at least in Python. This also allows for easier testing of the functionality both in terms of writing tests and manual testing in case of debugging an issue.


:return None/tuple:

None: topology isn't available for vector map

tuple: (number of zero lines, number of zero boundaries)
"""

map_name, mapset = ParseMapStr(map_name)

map = pointer(Map_info())
ret = Vect_open_old(map,
c_char_p(grass.encode(map_name)),
c_char_p(grass.encode(mapset)))
if ret == 1:
Vect_close(map)
if ret != 2:
return None

points = Vect_new_line_struct()
cats = Vect_new_cats_struct()
n_zero_lines = n_zero_boundaries = 0
nlines = Vect_get_num_lines(map)
for line in range(1, nlines + 1):
if not Vect_line_alive(map, line):
continue
ltype = Vect_get_line_type(map, line)
if ltype <= GV_LINES:
Vect_read_line(map, points, cats, line)
length = Vect_line_length(points)
if length == 0:
if ltype == GV_LINES:
n_zero_lines += 1
elif ltype == GV_BOUNDARY:
n_zero_boundaries +=1
Vect_close(map)
return n_zero_lines, n_zero_boundaries

def _cleanZeroLength(self, map_name):
"""Clean lines or boundaries of zero length
Comment on lines +911 to +912
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of border line case here. This calls modules, so it is okay in GUI in general and there is already v.build call here, but on the other hand, this file is toolbars.py. Something to consider. Maybe refactoring is needed if there is more stuff needed than just one v.build call.


:param str map_name: full vector map name
"""

output = grass.tempname(length=12, lowercase=True)
RunCommand('v.clean', type='line,boundary', tool='rmline',
thres=0.00, input=map_name,
output=output, quiet=True)
RunCommand('g.rename',
vector=[output, map_name.split('@')[0]],
quiet=True, overwrite=True)

def StartEditing(self, mapLayer):
"""Start editing selected vector map layer.

Expand All @@ -882,6 +946,43 @@ def StartEditing(self, mapLayer):
else:
return

# check and clean zero length lines, boundaries
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is already long, but if we add all these lines with two new return statements and at least six branches, it will be only harder to maintain later. If you stay with this solution, I suggest refactoring or shortening the code needed right here.

zero_length = self._topoZeroLengthCheck(
map_name=mapLayer.GetName(),
)

if zero_length is None:
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one return statement above returns None, but the three down below return boolean. Pylint should give a warning about that.

else:
ltype = None
if zero_length[0] > 0 and zero_length[1] == 0:
ltype = 'line(s)'
elif zero_length[1] > 0 and zero_length[0] == 0:
ltype = 'boundary(ies)'
elif zero_length[0] > 0 and zero_length[1] > 0:
ltype = 'line(s)/boundary(ies)'

if ltype:
clean_dlg = wx.MessageDialog(
self.GetParent(),
message=_(
"The vector map contains zero-length {ltype}, "
"to continue editing, you need to clean them. "
"Do you want to clean them?".format(ltype=ltype)
),
caption=_('Clean?'),
style=wx.YES_NO | wx.YES_DEFAULT | wx.ICON_QUESTION,
)
Comment on lines +957 to +975
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Translatable strings can't have sentences constructed from words. You need multiple messages or part of the message which is not a sentence but some {x}: {y} type of report or table. There is also a way how to change a message with support of the gettext package based on the number, but I don't know how to easily use it in Python with our suboptimal translation handling (that is _ added to buildins).

if not clean_dlg.ShowModal() == wx.ID_YES:
clean_dlg.Destroy()
return

clean_dlg.Destroy()

wait = wx.BusyCursor()
self._cleanZeroLength(map_name=mapLayer.GetName())
del wait

# deactive layer
self.Map.ChangeLayerActive(mapLayer, False)

Expand Down