Code review comment for lp:~doxxx/qbzr/mergetools

Revision history for this message
Gary van der Merwe (garyvdm) wrote :

Sorry about the delay in looking at this. Been busy at work.

Thank you for tackling this.

* qconfig and qconflicts break if using using a old bzr. I would like to keep some level of backwards compatibility. So if running an old bzr, qconfig should load, but the merge tab should show a error message. qconflicts should load, show conflicts, but you won't be able to launch a ext merges app.

* I would prefer for the qconfig merge tab to be a simple list, like the diff tab, rather than the list + form. My reasons for this are:
  + It's easier to code. With the list + form code, you have to do a lot of work to synchronize the selection in the list, and the form item displayed. (see bug below.)
  + Less widgets = Cleaner look.

Anyway - this is bike shedding. So if you disagree, I don't mind. What I do want is for the diff and merge tabs to share the same design, and code.

* I would be nice to give help on then merge parameters (%b %T %o, etc..). Maybe a link to qhelp, like the qgetnew dialog.

* First time I ran qconfig, I got the following error, because I did not have a default set.

bzr: ERROR: exceptions.IndexError: list index out of range

Traceback (most recent call last):
  File "/home/garyvdm/qbzr/mergetools/lib/commands.py", line 166, in run
    ret_code = self._qbzr_run(*args, **kwargs)
  File "/home/garyvdm/qbzr/mergetools/lib/commands.py", line 504, in _qbzr_run
    window = QBzrConfigWindow()
  File "/home/garyvdm/bzr/mergetools/bzrlib/lazy_import.py", line 128, in __call__
    return obj(*args, **kwargs)
  File "/home/garyvdm/qbzr/mergetools/lib/config.py", line 247, in __init__
    self.load()
  File "/home/garyvdm/qbzr/mergetools/lib/config.py", line 378, in load
    self.merge_tools_list_model.set_merge_tools(definedMergeTools, defaultMergeTool)
  File "/home/garyvdm/qbzr/mergetools/lib/config.py", line 743, in set_merge_tools
    self._default = [mt for mt in self._merge_tools if mt == default][0]
IndexError: list index out of range

I've fixed this and pushed to lp:~qbzr-dev/qbzr/mergetools. I've added you to ~qbzr-dev, so if you want, you can also push there.

* In qconfig, if you click on then ... button for the command line, and click cancel, it changes the command line. It should not.

* I saw this bug: steps to reproduce:
Click on a merge tool.
Click detect. (There is now no selection in the list box, but the form on the left stays populated.)
Edit the command line.
press tab (or otherwise let the command line text box lose focus.)

Actual result: get this error:
bzr: ERROR: exceptions.AssertionError:

Traceback (most recent call last):
  File "/home/garyvdm/qbzr/mergetools/lib/config.py", line 606, in merge_tool_name_changed
    assert sel_model.hasSelection()
AssertionError

* The qconficts code seems pretty solid.

I've been wanting to change qconficts to use the treewidget for a while. I'll see if I can work on it shortly.

I also want to make a merge tool menu, like then diff tool menu, so you can select the merge tool in qcommit, qbrowse, be wt view, etc.

review: Needs Fixing

« Back to merge proposal