Merge lp:~craighewetson-deactivatedaccount/bzr-explorer/shorten-refresh-time into lp:bzr-explorer

[Craig Hewetson]

After every action in bzr explorer the working tree gets refreshed and for large trees this makes the ui rather unresponsive. So instead of initiating a refresh of the working tree
after the user closes an action's dialog. The refresh will take place once the action has completed. If this is done the time between the user realizing that the action has completed
and when user closes the dialog can now be used for refreshing the working tree.
This means the user will effectively have to wait less for refreshes of the working tree and hence the app will feel more responsive for working larger trees.

[Craig Hewetson (craighewetson-deactivatedaccount)]

This feature comes from the email discussion on how to reduce the time taken from when a qbzr dialog is closed to when the working tree is refreshed.

email subject:
Bzr-Explorer: Embedding status output after executing qbzr dialogs.

The only dialogs I didn't do is:
qconflicts, pull, push and merge.

[Craig Hewetson (craighewetson-deactivatedaccount)]

My only problem i have is that i would like to call self.do_refresh() from a separate Thread. Because the dialog now takes longer to "complete" since its holding the ui to do the refresh work.

I'm not a python pro so is there any way to call the do_refresh() method from a Thread so that the ui does not get held up.
I've tried but I get all sorts of errors.
(QObject: Cannot create children for a parent that is in a different thread.
(Parent is QTextDocument(0x9cb6b18), parent's thread is QThread(0x98f7f00), current thread is QThread(0x9e58db8))

In java I'll easily be able to do this, but thats just because I know java better.

[Alexander Belchenko (bialix)]

Craig Hewetson пишет:
> My only problem i have is that i would like to call self.do_refresh() from a separate Thread. Because the dialog now takes longer to "complete" since its holding the ui to do the refresh work.

In PyQt you should not call methods directly but instead you should send
a signal to do what you need. Signals are asynchronous, and it won't
block your dialog.

[Craig Hewetson (craighewetson-deactivatedaccount)]

>
>
> In PyQt you should not call methods directly but instead you should send
> a signal to do what you need. Signals are asynchronous, and it won't
> block your dialog.
>
>

It seems the bzr explorer already listeners to the finished() signal when
showing the dialog. The following code snippet comes from explorer.py line
1388 to 1396:

                    if finisher:
                        # We keep track of the most recently opened dialog
for
                        # each command-id so we can find it during the
finisher.
                        self._custom_modeless_dialogs[cmd_id] = dialog
                        # NOTE: This isn't actually the close event but the
                        # subprocess competion event. We may want to tweak
                        # that in the future.
                        self.connect(dialog.process_widget,
                            QtCore.SIGNAL("finished()"), finisher)

I assume from this that the finisher method gets called when the
"finished()" signal is sent. But the dialog seems to get blocked i.e the OK
and Cancel buttons take a while before they get replaced with the "Close"
button.

Its very likely that I'm missing something here...

[Alexander Belchenko (bialix)]

> This feature comes from the email discussion on how to reduce the time taken
> from when a qbzr dialog is closed to when the working tree is refreshed.
>
> email subject:
> Bzr-Explorer: Embedding status output after executing qbzr dialogs.

The discussion is here: http://groups.google.com/group/qbzr/browse_thread/thread/274cf3859382f928#

[Alexander Belchenko (bialix)]

What is the status of this merge proposal today?

review: Needs Information

[Craig Hewetson (craighewetson-deactivatedaccount)]

This branch isn't ready yet, I would have to find some time to look at it
again. I think i had an issue with the "finished()" signal... it was being
called too late

On Tue, Aug 24, 2010 at 7:57 PM, Alexander Belchenko <email address hidden> wrote:

> Review: Needs Information
> What is the status of this merge proposal today?
> --
>
> https://code.launchpad.net/~craighewetson/bzr-explorer/shorten-refresh-time/+merge/18822
> You are the owner of lp:~craighewetson/bzr-explorer/shorten-refresh-time.
>

[Craig Hewetson (craighewetson-deactivatedaccount)]

I've found a problem and it has to do with qbzr.

The SubProcessWindowBase class has 3 buttons (ok, cancel and close) and these buttons change state or get hidden/shown when the "subprocessFinished(bool)" signal gets emitted which only get emitted after the "finished()" signal gets emitted.

The problem is that when I connect with the "finished()" from bzr-explorer to run a refresh of the working tree. But I want the "close" button to show BEFORE my refresh work takes place. Currently it only gets shown AFTER the refresh has completed thus the dialog stays open while the working tree gets refreshed and this defeats the purpose of my feature.

I found a workaround to this and it requires me to change the do_finish method in SubProcessWindowBase to look the like the following:

    def on_finished(self):
        if hasattr(self, 'setResult'):
            self.setResult(QtGui.QDialog.Accepted)

        self.emit(QtCore.SIGNAL("subprocessFinished(bool)"), True)
        self.emit(QtCore.SIGNAL("disableUi(bool)"), False)

        self.return_code = 0

        if not self.ui_mode:
            self.close()
        else: # This is where the change happens
            self.okButton.setHidden(True)
            self.closeButton.setHidden(False)

NOTE: I explicitly change the state of the buttons while not solely relying on the "subprocessFinished(bool)" signal to change the buttons states... Also it would mean that the buttons would have to be accessible from the method. okButton => self.okButton

This does feel like a hack so if there is a better suggestion please let me know ...

[Alexander Belchenko (bialix)]

See related proposal where Simon did similar changes (okButton => self.okButton) https://code.launchpad.net/~simon-kersey/qbzr/590229-fix/+merge/33901

[Alexander Belchenko (bialix)]

> This does feel like a hack so if there is a better suggestion please let me know ...

What do you mean by "this"?

[Craig Hewetson (craighewetson-deactivatedaccount)]

The idea of directly hiding and showing buttons and allowing the signal to
be emitted to do the same thing.
BUT, based on the comments on Simon's merge proposal and with me rethinking
my comment, I think its probably not a hack.

So I'll make the change and propose a merge...
Thanks

[Craig Hewetson (craighewetson-deactivatedaccount)]

This patch will still will perform better when the code changes from this branch:
lp:~craighewetson/qbzr/subprocess_buttons_statechange
is merged into the qbzr project.

Note these changes can still be merged into bzr-explorer without the qbzr changes, because it will only result in the user having to wait for the subprocess dialog's close button getting enabled while the working tree is refreshed.

Once the qbzr change is merged the dialog can be closed as soon as the subprocess is finished and even while the refresh is taking place.

review: Needs Resubmitting

[Alexander Belchenko (bialix)]

Craig, why you voted "Resubmit"? In Launchpad terms it means you asking yourself to rework the patch. Instead you should change status of proposal from "Work In Progress" to "Needs Review".

Anyway, I need something to put into NEWS file about this improvement.

Also, your changes for QBzr will be merged into QBzr 0.20. If it helps you can check the version of QBzr and don't block it if QBzr version < 0.20?

Another question: while Close button is not active user should still be able to close the dialog with Alt+F4 or Close icon in the dialog title. Can you check that it still works?

review: Needs Fixing

[Alexander Belchenko (bialix)]

Craig, I think I know how to not block Close button.
You should not execute self.do_refresh() immediately, instead you should send additional signal to yourself with

QtCore.QTimer.singleShot(1, self.do_refresh)

The timer will start refresh once all other signals will be processed, as I understand. So buttons should be activated and won't blocked by refresh.

Craig, please test this change and say if it works as expected without patching QBzr.

review: Needs Information

[Alexander Belchenko (bialix)]

@Craig, we need to decide something about this patch.

[Craig Hewetson (craighewetson-deactivatedaccount)]

Hi Alexander
The problem is that none of the solutions actually freed up the UI thread enough to allow the user to close the dialog while the refresh is taking place. So to do the refresh after the dialog closes (currently happens in released version) is probably better than doing the refresh before its closed (this is what currently happens in this branch).

I don't think we can use this patch now. Until we can get bzr-explorer to call "refresh" (access bzrlib) from another thread other than the UI Thread.

[Alexander Belchenko (bialix)]

I'm marking this merge proposal as Rejected for now, in hope we invent better solution later.

Unmerged revisions

486. By Craig Hewetson on 2010-09-03

Refresh the bzr-explorer working tree once the "custom" qbzr dialogs have completed their specific operations.

=== modified file 'lib/custom_dialogs.py'
--- lib/custom_dialogs.py	2010-02-04 08:55:56 +0000
+++ lib/custom_dialogs.py	2010-09-03 21:53:39 +0000
@@ -36,6 +36,11 @@
 import workspace_dialogs
 import checkout_dialog
 import switch_dialog
+from bzrlib.plugins.qbzr.lib.update import QBzrUpdateWindow
+from bzrlib.plugins.qbzr.lib.add import AddWindow
+from bzrlib.plugins.qbzr.lib.commit import CommitWindow
+from bzrlib.plugins.qbzr.lib.revert import RevertWindow
+from bzrlib.plugins.qbzr.lib.uncommit import QBzrUncommitWindow
 
 class CustomDialogRegistry(registry.Registry):
     """Registry for custom dialogs.
@@ -54,3 +59,8 @@
         checkout_dialog.QCheckoutExplorerStyleDialog)
 custom_dialog_registry.register('switch',
         switch_dialog.QSwitchExplorerStyleDialog)
+custom_dialog_registry.register('update', QBzrUpdateWindow)
+custom_dialog_registry.register('add', AddWindow)
+custom_dialog_registry.register('commit', CommitWindow)
+custom_dialog_registry.register('revert', RevertWindow)
+custom_dialog_registry.register('uncommit', QBzrUncommitWindow)
 
=== modified file 'lib/explorer.py'
--- lib/explorer.py	2010-08-24 17:34:38 +0000
+++ lib/explorer.py	2010-09-03 21:53:39 +0000
@@ -26,6 +26,7 @@
     trace,
     urlutils,
     )
+from bzrlib.workingtree import WorkingTree
 from bzrlib.commands import shlex_split_unicode
 from bzrlib.plugins import explorer
 from bzrlib.plugins.explorer.lib import (
@@ -94,12 +95,11 @@
 
 # Actions that should automatically refresh the page on successful completion
 _AUTO_REFRESH_CMDS = [
-    'add', 'commit',
-    'pull', 'merge', 'update', 'push', 'send',
-    'conflicts', 'switch', 'bind', 'unbind', 'revert', 'uncommit',
-    # This one really ought to be handled by a custom dialog so that the new
+    'send', 'bind', 'unbind', 'conflicts', 'pull', 'merge', 'push',
+    # These really ought to be handled by a custom dialog so that the new
     # location can be opened. Refreshing the current page (often the
     # repository view) is better than nothing though.
+    'add', 'commit', 'switch', 'update', 'revert', 'uncommit',
     'branch',
     ]
 
@@ -1430,6 +1430,11 @@
             'branch': (self._start_branch, False, self._finish_branch),
             'checkout': (self._start_checkout, False, self._finish_checkout),
             'switch': (self._start_switch, False, self._finish_switch),
+            'update': (self._start_update, False, self._finish_do_refresh),
+            'add': (self._start_add, False, self._finish_do_refresh),
+            'commit': (self._start_commit, False, self._finish_do_refresh),
+            'revert': (self._start_revert, False, self._finish_do_refresh),
+            'uncommit': (self._start_uncommit, False, self._finish_do_refresh),
             }
         if cmd_id in internal_handlers and cmd_id in custom_dialog_registry:
             if self.dry_run:
@@ -1497,6 +1502,38 @@
             new_location = osutils.abspath(to_location)
             self.open_location(new_location)
 
+    def _start_update(self, context=None):
+        location = context.get('branch_root')
+        tree = WorkingTree.open_containing(location)[0]
+        dialog = custom_dialog_registry.get('update')
+        return dialog(tree, parent=self)
+    
+    def _start_add(self, context=None):
+        location = context.get('branch_root')
+        tree = WorkingTree.open_containing(location)[0]
+        dialog = custom_dialog_registry.get('add')
+        return dialog(tree, None, parent=self)
+    
+    def _start_commit(self, context=None):
+        location = context.get('branch_root')
+        tree = WorkingTree.open_containing(location)[0]
+        dialog = custom_dialog_registry.get('commit')
+        return dialog(tree, None, parent=self)
+    
+    def _start_revert(self, context=None):
+        location = context.get('branch_root')
+        tree = WorkingTree.open_containing(location)[0]
+        dialog = custom_dialog_registry.get('revert')
+        return dialog(tree, None, parent=self)
+    
+    def _start_uncommit(self, context=None):
+        location = context.get('branch_root')
+        dialog = custom_dialog_registry.get('uncommit')
+        return dialog(location, parent=self)
+
+    def _finish_do_refresh(self):
+        self.do_refresh()
+
     def _start_checkout(self, context=None):
         return custom_dialog_registry.get('checkout')(parent=self)
 
 
=== modified file 'lib/explorer_preferences.py'
--- lib/explorer_preferences.py	2010-08-06 15:08:29 +0000
+++ lib/explorer_preferences.py	2010-09-03 21:53:39 +0000
@@ -43,6 +43,11 @@
     "custom-dialog-branch": True,
     "custom-dialog-checkout": True,
     "custom-dialog-switch": True,
+    "custom-dialog-update": True,
+    "custom-dialog-add": True,
+    "custom-dialog-commit": True,
+    "custom-dialog-revert": True,
+    "custom-dialog-uncommit": True,
     "advanced-commands-terminal": False,
     "workingtree-style": "qbrowse",
     "workspace-model": "feature-branches",
@@ -65,6 +70,7 @@
     "custom-dialog-branch",
     "custom-dialog-checkout",
     "custom-dialog-switch",
+    "custom-dialog-update",
     "advanced-commands-terminal",
     "bind-branches-by-default",
     "toolbox-on-status-view",