wxGUI/startup: fix terminate location download dialog download thread (#832) (#873)

* wxGUI/startup: don't set message wx widget string if location download dialog is destroyed
* wxGUI/core: replace set/reset stop flag for terminate thread with traces (urlretrieve runned from thread can't terminate with stop flag)
* wxGUI/startup: avoid printing annoying wx debug message on location download dialog start-up
* wxGUI/startup: change 'Download' button label to 'Abort' after start downloading, and allow terminate download thread correctly
* wxGUI/startup: handle location download dialog urlretrieve HTTPError, URLError exception
* wxGUI/startup: add string translation function
* wxGUI/startup: set location download dialog download button mnemonic label
* wxGUI/startup: fix close location download modal dialog
* wxGUI/startup: fix location download dialog 'Cancel' and 'Download' button layout
* wxGUI/startup: centre location download modal dialog

gui/wxpython/core/gthread.py

@@ -18,6 +18,7 @@ import threading
 import time
 
 import wx
+from wx.lib.newevent import NewEvent
 
 import sys
 if sys.version_info.major == 2:
@@ -27,14 +28,21 @@ else:
 
 from core.gconsole import EVT_CMD_DONE, wxCmdDone
 
+wxThdTerminate, EVT_THD_TERMINATE = NewEvent()
+
 
 class gThread(threading.Thread, wx.EvtHandler):
-    """Thread for various backends"""
+    """Thread for various backends
+
+    terminating thread:
+    https://www.geeksforgeeks.org/python-different-ways-to-kill-a-thread/
+    """
     requestId = 0
 
     def __init__(self, requestQ=None, resultQ=None, **kwds):
         wx.EvtHandler.__init__(self)
         self.terminate = False
+        self._terminate_evt = None
 
         threading.Thread.__init__(self, **kwds)
 
@@ -51,6 +59,7 @@ class gThread(threading.Thread, wx.EvtHandler):
         self.setDaemon(True)
 
         self.Bind(EVT_CMD_DONE, self.OnDone)
+        self.Bind(EVT_THD_TERMINATE, self.OnTerminate)
         self.start()
 
     def Run(self, *args, **kwds):
@@ -80,7 +89,7 @@ class gThread(threading.Thread, wx.EvtHandler):
     def run(self):
         while True:
             requestId, args, kwds = self.requestQ.get()
-            for key in ('callable', 'ondone', 'userdata'):
+            for key in ('callable', 'ondone', 'userdata', 'onterminate'):
                 if key in kwds:
                     vars()[key] = kwds[key]
                     del kwds[key]
@@ -93,6 +102,13 @@ class gThread(threading.Thread, wx.EvtHandler):
             exception = None
             time.sleep(.01)
 
+            self._terminate_evt = wxThdTerminate(
+                onterminate=vars()['onterminate'],
+                kwds=kwds,
+                args=args,
+                pid=requestId,
+            )
+
             if self.terminate:
                 return
 
@@ -123,3 +139,31 @@ class gThread(threading.Thread, wx.EvtHandler):
     def Terminate(self, terminate=True):
         """Abort command(s)"""
         self.terminate = terminate
+
+    def start(self):
+        self.__run_backup = self.run
+        self.run = self.__run
+        threading.Thread.start(self)
+
+    def __run(self):
+        sys.settrace(self.globaltrace)
+        self.__run_backup()
+        self.run = self.__run_backup
+
+    def globaltrace(self, frame, event, arg):
+        if event == 'call':
+            return self.localtrace
+        else:
+            return None
+
+    def localtrace(self, frame, event, arg):
+        if self.terminate:
+            if event == 'line':
+                # Send event
+                wx.PostEvent(self, self._terminate_evt)
+                raise SystemExit()
+        return self.localtrace
+
+    def OnTerminate(self, event):
+        if event.onterminate:
+            event.onterminate(event)

gui/wxpython/gis_set.py

@@ -33,6 +33,11 @@ from grass.script import core as grass
 
 from core import globalvar
 import wx
+# import adv and html before wx.App is created, otherwise
+# we get annoying "Debug: Adding duplicate image handler for 'Windows bitmap file'"
+# during download location dialog start up, remove when not needed
+import wx.adv
+import wx.html
 import wx.lib.mixins.listctrl as listmix
 
 from core.gcmd import GMessage, GError, DecodeString, RunCommand
@@ -766,6 +771,7 @@ class GRASSStartup(wx.Frame):
         from startup.locdownload import LocationDownloadDialog
 
         loc_download = LocationDownloadDialog(parent=self, database=self.gisdbase)
+        loc_download.Centre()
         loc_download.ShowModal()
         location = loc_download.GetLocation()
         if location:

gui/wxpython/startup/locdownload.py

@@ -22,15 +22,17 @@ import os
 import sys
 import tempfile
 import shutil
+import textwrap
 import time
 
 try:
     from urllib2 import HTTPError, URLError
-    from urllib import urlopen, urlretrieve
+    from urllib import request, urlopen, urlretrieve
 except ImportError:
     # there is also HTTPException, perhaps change to list
     from urllib.error import HTTPError, URLError
     from urllib.request import urlopen, urlretrieve
+    from urllib import request
 
 import wx
 from wx.lib.newevent import NewEvent
@@ -87,13 +89,18 @@ class DownloadError(Exception):
     """Error happened during download or when processing the file"""
     pass
 
+
 class RedirectText(object):
     def __init__(self, window):
         self.out = window
 
     def write(self, string):
         try:
-            wx.CallAfter(self.out.SetLabel, string)
+            if self.out:
+                string = self._wrap_string(string)
+                heigth = self._get_heigth(string)
+                wx.CallAfter(self.out.SetLabel, string)
+                self._resize(heigth)
         except:
             # window closed -> PyDeadObjectError
             pass
@@ -101,6 +108,43 @@ class RedirectText(object):
     def flush(self):
         pass
 
+    def _wrap_string(self, string, width=40):
+        """Wrap string
+
+        :param str string: input string
+        :param int width: maximum length allowed of the wrapped lines
+
+        :return str: newline-separated string
+        """
+        wrapper = textwrap.TextWrapper(width=width)
+        return wrapper.fill(text=string)
+
+    def _get_heigth(self, string):
+        """Get widget new heigth
+
+        :param str string: input string
+
+        :return int: widget heigth
+        """
+        n_lines = string.count('\n')
+        attr = self.out.GetClassDefaultAttributes()
+        font_size = attr.font.GetPointSize()
+        heigth = int((n_lines + 2) * font_size // 0.75) # 1 px = 0.75 pt
+        return heigth
+
+    def _resize(self, heigth=-1):
+        """Resize widget heigth
+
+        :param int heigth: widget heigth
+        """
+        wx.CallAfter(self.out.GetParent().SetMinSize, (-1, -1))
+        wx.CallAfter(self.out.SetMinSize, (-1, heigth))
+        wx.CallAfter(
+            self.out.GetParent().parent.sizer.Fit,
+            self.out.GetParent().parent,
+        )
+
+
 # copy from g.extension, potentially move to library
 def move_extracted_files(extract_dir, target_dir, files):
     """Fix state of extracted file by moving them to different diretcory
@@ -177,7 +221,9 @@ def reporthook(count, block_size, total_size):
     global start_time
     if count == 0:
         start_time = time.time()
-        sys.stdout.write("Download in progress, wait until it is finished\n0%")
+        sys.stdout.write(
+            _('Download in progress, wait until it is finished 0%'),
+        )
         return
     if count % 100 != 0: # be less verbose
         return
@@ -185,9 +231,13 @@ def reporthook(count, block_size, total_size):
     progress_size = int(count * block_size)
     speed = int(progress_size / (1024 * duration))
     percent = int(count * block_size * 100 / total_size)
-    sys.stdout.write("Download in progress, wait until it is finished\n{0}%, {1} MB, {2} KB/s, {3:.0f} seconds passed".format(
-        percent, progress_size / (1024 * 1024), speed, duration
-    ))
+    sys.stdout.write(
+        _("Download in progress, wait until it is finished "
+          "{0}%, {1} MB, {2} KB/s, {3:.0f} seconds passed".format(
+              percent, progress_size / (1024 * 1024), speed, duration,
+          ),
+        ),
+    )
 
 # based on g.extension, potentially move to library
 def download_and_extract(source):
@@ -195,9 +245,27 @@ def download_and_extract(source):
     tmpdir = tempfile.mkdtemp()
     Debug.msg(1, 'Tmpdir: {}'.format(tmpdir))
     directory = os.path.join(tmpdir, 'location')
+    http_error_message = _(
+        "Download file from <{url}>, "
+        "return status code {code}, "
+    )
+    url_error_message = _(
+        "Download file from <{url}>, "
+        "failed. Check internet connection."
+    )
     if source.endswith('.zip'):
         archive_name = os.path.join(tmpdir, 'location.zip')
-        filename, headers = urlretrieve(source, archive_name, reporthook)
+        try:
+            filename, headers = urlretrieve(source, archive_name, reporthook)
+        except HTTPError as err:
+            raise DownloadError(
+                http_error_message.format(
+                        url=source,
+                        code=err,
+                ),
+            )
+        except URLError:
+            raise DownloadError(url_error_message.format(url=source))
         if headers.get('content-type', '') != 'application/zip':
             raise DownloadError(
                 _("Download of <{url}> failed"
@@ -211,8 +279,17 @@ def download_and_extract(source):
         else:
             ext = source.rsplit('.', 1)[1]
         archive_name = os.path.join(tmpdir, 'location.' + ext)
-        urlretrieve(source, archive_name, reporthook)
-        # TODO: error handling for urlretrieve
+        try:
+            urlretrieve(source, archive_name, reporthook)
+        except HTTPError as err:
+            raise DownloadError(
+                http_error_message.format(
+                    url=source,
+                    code=err,
+                ),
+            )
+        except URLError:
+            raise DownloadError(url_error_message.format(url=source))
         extract_tar(name=archive_name, directory=directory, tmpdir=tmpdir)
     else:
         # probably programmer error
@@ -285,10 +362,13 @@ class LocationDownloadPanel(wx.Panel):
         """
         wx.Panel.__init__(self, parent=parent)
 
+        self.parent = parent
         self._last_downloaded_location_name = None
         self._download_in_progress = False
         self.database = database
         self.locations = locations
+        self._abort_btn_label = _('Abort')
+        self._abort_btn_tooltip = _('Abort download location')
 
         self.label = StaticText(
             parent=self,
@@ -300,11 +380,7 @@ class LocationDownloadPanel(wx.Panel):
         self.choice = wx.Choice(parent=self, choices=choices)
 
         self.choice.Bind(wx.EVT_CHOICE, self.OnChangeChoice)
-
-        self.download_button = Button(parent=self, id=wx.ID_ANY,
-                                      label=_("Do&wnload"))
-        self.download_button.SetToolTip(_("Download selected location"))
-        self.download_button.Bind(wx.EVT_BUTTON, self.OnDownload)
+        self.parent.download_button.Bind(wx.EVT_BUTTON, self.OnDownload)
         # TODO: add button for a link to an associated website?
         # TODO: add thumbnail for each location?
 
@@ -338,13 +414,6 @@ class LocationDownloadPanel(wx.Panel):
                      flag=wx.EXPAND | wx.TOP | wx.LEFT | wx.RIGHT, border=10)
         vertical.Add(self.choice, proportion=0,
                      flag=wx.EXPAND | wx.TOP | wx.LEFT | wx.RIGHT, border=10)
-
-        button_sizer = wx.BoxSizer(wx.HORIZONTAL)
-        button_sizer.AddStretchSpacer()
-        button_sizer.Add(self.download_button, proportion=0)
-
-        vertical.Add(button_sizer, proportion=0,
-                     flag=wx.EXPAND | wx.TOP | wx.LEFT | wx.RIGHT | wx.ALIGN_RIGHT, border=10)
         vertical.AddStretchSpacer()
         vertical.Add(self.message, proportion=0,
                      flag=wx.ALIGN_CENTER_VERTICAL |
@@ -355,14 +424,28 @@ class LocationDownloadPanel(wx.Panel):
         self.Layout()
         self.SetMinSize(self.GetBestSize())
 
+    def _change_download_btn_label(self, label=_('Do&wnload'),
+                                tooltip=_('Download selected location')):
+        """Change download button label/tooltip"""
+        if self.parent.download_button:
+            self.parent.download_button.SetLabel(label)
+            self.parent.download_button.SetToolTip(tooltip)
+
     def OnDownload(self, event):
         """Handle user-initiated action of download"""
-        Debug.msg(1, "OnDownload")
-        if self._download_in_progress:
-            self._warning(_("Download in progress, wait until it is finished"))
-        index = self.choice.GetSelection()
-        self.DownloadItem(self.locations[index])
-        self.download_button.Enable(False)
+        button_label = self.parent.download_button.GetLabel()
+        if button_label in (_('Download'), _('Do&wnload')) :
+            self._change_download_btn_label(
+                label=self._abort_btn_label,
+                tooltip=self._abort_btn_tooltip,
+            )
+            Debug.msg(1, "OnDownload")
+            if self._download_in_progress:
+                self._warning(_("Download in progress, wait until it is finished"))
+            index = self.choice.GetSelection()
+            self.DownloadItem(self.locations[index])
+        else:
+            self.parent.OnCancel()
 
     def DownloadItem(self, item):
         """Download the selected item"""
@@ -374,6 +457,7 @@ class LocationDownloadPanel(wx.Panel):
         if os.path.exists(destination):
             self._error(_("Location named <%s> already exists,"
                           " download canceled") % dirname)
+            self._change_download_btn_label()
             return
 
         def download_complete_callback(event):
@@ -386,12 +470,21 @@ class LocationDownloadPanel(wx.Panel):
                 self._warning(_("Download completed. The downloaded sample data is listed "
                                 "in the location/mapset tabs upon closing of this window")
                 )
+            self._change_download_btn_label()
+
+        def terminate_download_callback(event):
+            self._download_in_progress = False
+            request.urlcleanup()
+            sys.stdout.write("Download aborted")
+            self.thread = gThread()
+            self._change_download_btn_label()
 
         self._download_in_progress = True
         self._warning(_("Download in progress, wait until it is finished"))
         self.thread.Run(callable=download_location,
                         url=url, name=dirname, database=self.database,
-                        ondone=download_complete_callback)
+                        ondone=download_complete_callback,
+                        onterminate=terminate_download_callback)
 
     def OnChangeChoice(self, event):
         """React to user changing the selection"""
@@ -407,6 +500,7 @@ class LocationDownloadPanel(wx.Panel):
         if os.path.exists(destination):
             self._warning(_("Location named <%s> already exists,"
                             " rename it first") % dirname)
+            self.parent.download_button.SetLabel(label=_('Download'))
             return
         else:
             self._clearMessage()
@@ -426,7 +520,7 @@ class LocationDownloadPanel(wx.Panel):
             _clearMessage() when you know that there is everything
             correct.
         """
-        self.message.SetLabel(text)
+        sys.stdout.write(text)
         self.sizer.Layout()
 
     def _error(self, text):
@@ -440,7 +534,7 @@ class LocationDownloadPanel(wx.Panel):
             _clearMessage() when you know that there is everything
             correct.
         """
-        self.message.SetLabel(_("Error: {text}").format(text=text))
+        sys.stdout.write(_("Error: {text}").format(text=text))
         self.sizer.Layout()
 
     def _clearMessage(self):
@@ -463,22 +557,37 @@ class LocationDownloadDialog(wx.Dialog):
         :param title: window title if the default is not appropriate
         """
         wx.Dialog.__init__(self, parent=parent, title=title)
+        cancel_button = Button(self, id=wx.ID_CANCEL)
+        self.download_button = Button(parent=self, id=wx.ID_ANY,
+                                      label=_("Do&wnload"))
+        self.download_button.SetToolTip(_("Download selected location"))
         self.panel = LocationDownloadPanel(parent=self, database=database)
-        close_button = Button(self, id=wx.ID_CLOSE)
-        # TODO: terminate download process
-        close_button.Bind(wx.EVT_BUTTON, self.OnClose)
+        cancel_button.Bind(wx.EVT_BUTTON, self.OnCancel)
+        self.Bind(wx.EVT_CLOSE, self.OnCancel)
 
-        sizer = wx.BoxSizer(wx.VERTICAL)
-        sizer.Add(self.panel, proportion=1, flag=wx.EXPAND)
+        self.sizer = wx.BoxSizer(wx.VERTICAL)
+        self.sizer.Add(self.panel, proportion=1, flag=wx.EXPAND)
 
         button_sizer = wx.StdDialogButtonSizer()
-        button_sizer.Add(close_button)
+        button_sizer.Add(
+            cancel_button,
+            proportion=0,
+            flag=wx.EXPAND | wx.LEFT | wx.RIGHT,
+            border=5,
+        )
+        button_sizer.Add(
+            self.download_button,
+            proportion=0,
+            flag=wx.EXPAND | wx.LEFT | wx.RIGHT,
+            border=5,
+        )
         button_sizer.Realize()
 
-        sizer.Add(button_sizer, proportion=0,
-                  flag=wx.ALIGN_RIGHT | wx.BOTTOM, border=10)
-        self.SetSizer(sizer)
-        sizer.Fit(self)
+        self.sizer.Add(button_sizer, proportion=0,
+                       flag=wx.ALIGN_RIGHT | wx.TOP | wx.BOTTOM,
+                       border=10)
+        self.SetSizer(self.sizer)
+        self.sizer.Fit(self)
 
         self.Layout()
 
@@ -486,7 +595,7 @@ class LocationDownloadDialog(wx.Dialog):
         """Get the name of the last location downloaded by the user"""
         return self.panel.GetLocation()
 
-    def OnClose(self, event):
+    def OnCancel(self, event=None):
         if self.panel._download_in_progress:
             # running thread
             dlg = wx.MessageDialog(parent=self,
@@ -498,11 +607,15 @@ class LocationDownloadDialog(wx.Dialog):
             ret = dlg.ShowModal()
             dlg.Destroy()
 
-            # TODO: terminate download process on wx.ID_YES
             if ret == wx.ID_NO:
                 return
+            else:
+                self.panel.thread.Terminate()
+                self.panel._change_download_btn_label()
+
+        if event:
+            self.EndModal(wx.ID_CANCEL)
 
-        self.Close()
 
 def main():
     """Tests the download dialog"""