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

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

No worries, thanks for the review.

> * 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.

Okay.

> * 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 went with a separate list and edit form design because I thought it would be easier to use, which I felt was a drawback of the previous design. For instance, there's no obvious clue that the radio button of the previous design indicates that it marks the default tool.

I will investigate an alternate design using a table though, to see if I can come up with something that is easier to use than the old design and can be re-used for the Diff tab.

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

Okay.

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

> 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.

Cool. Thanks. Can I just pull/push --remember to that with my current branch and continue working like that? And if I do, should I resubmit this merge proposal using the new branch?

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

Indeed 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

Yeah, that's to catch code paths like the one you discovered when the list of merge tools has no selection but the edit form is still enabled. I'll investigate.

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

You mean a treewidget to show the conflicts in the context of the working tree structure?

> 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.

Nice idea.