Re: [PATCH] auto merge action

[Kai Willadsen <kai willadsen gmail com>]

On 28 April 2010 22:59, Piotr Piastucki <leech miranda gmail com> wrote:
> Hi,
>
> The following patch adds 3 new merge action to the context menu. The actions
> work in 3-way diff mode only and they respectively apply or merge all
> non-conflicting changes in one shot. Comments appreciated.

So firstly: in general these actions are a really nice idea. I think
this would also let us completely remove the 'Copy All to Left/Right'
actions, since the 'Pull Non-conflicting' actions do a better job in
95% of cases. For the remaining cases, I think people can live with
select-all-copy-paste.

As a general comment, can you please double-check your code for
correct (PEP8) spacing. There's some unnecessary spacing around
braces, and missing spacing around operators and commas.

     <separator/>
+    <menuitem action="MergeNonConflicting"/>
+    <menuitem action="PullNonConflictingLeft"/>
+    <menuitem action="PullNonConflictingRight"/>
+    <separator/>
     <menuitem action="FileOpen" />
   </popup>

I know that Meld doesn't obey this currently, but for new UI
additions, we should be following the rule of having all actions
available through the menus, even if they're also available on the
toolbar/a popup. Also, I'm not convinced that these should even *be*
in the pane popup; the non-conflicting merge definitely shouldn't be,
IMO, since it operates on the middle pane, regardless of which pane
you open the context menu in.

Relatedly, I've been thinking about creating a new top-level menu
"Changes" to hold all actions related to manipulating or navigating
chunks. Currently this includes: Previous, Next, Pull left/right, Push
left/right and Delete. Your merge-y actions could easily fit in below
these in a new section.

             ("CopyAllRight",      gtk.STOCK_GOTO_LAST,  _("Copy To
Right"), None, _("Copy all changes from left pane to right pane"),
lambda x: self.copy_selected(1)),
+            ("MergeNonConflicting",      None,  _("Merge all
non-conflicting"), None, _("Merge all non-conflicting changes from
left and right pane"), lambda x:
self.merge_all_non_conflicting_changes()),
+            ("PullNonConflictingLeft",   None, _("Pull all
non-conflicting from left"),  None, _("Pull all non-conflicting
changes from right pane to left pane"), lambda x:
self.pull_all_non_conflicting_changes(-1)),
+            ("PullNonConflictingRight",  None,  _("Pull all
non-conflicting from right"), None, _("Pull all non-conflicting
changes from left pane to right pane"), lambda x:
self.pull_all_non_conflicting_changes(1)),

I don't have any better suggestions right now, but these actions could
use better strings.

+    def pull_all_non_conflicting_changes(self, direction):
+        assert direction in (-1,1)
+        dst = self._get_focused_pane()
+        src = dst + direction
+        assert src in range(self.num_panes)
+        filteredpanetext = [t for t in self._get_texts()]
+        merger = merge.Merger();
+        merger.differ = self.linediffer
+        merger.texts = filteredpanetext
+        for mergedfile in merger.merge_2_files(src, dst):
+            pass
+        self.on_textbuffer__begin_user_action()
+        self.textbuffer[dst].set_text(mergedfile)
+        self.on_textbuffer__end_user_action()
+        self.scheduler.add_task( lambda : self._sync_vscroll(
self.scrolledwindow[src].get_vadjustment(), src ) and None )

I may be missing something, but I don't really see how the result of
this is different to iterating over the existing change sequences in
Differ and applying all non-conflict changes (using
FileDiff.replace_chunk/delete_chunk). In fact, this may be worse, as
Differ and Merger can (IIRC) generate different results, and then we'd
be applying different changes to the ones we showed the user.

As an aside, there is no check here for whether anything can be/was
actually applied. If you try to pull from a side with no changes (or
all conflicts) then the whole buffer will be reset, the modified
status will be set, etc. It would be better if, in these cases, the
action was insensitive. That comes back to using the existing results
from Differ, however.

+    def merge_all_non_conflicting_changes(self):
<snip>

Same comment as above re: no-op buffer changes and sensitivity setting.

     def popup_in_pane(self, pane):
-        self.actiongroup.get_action("CopyAllLeft").set_sensitive(pane > 0)
-        self.actiongroup.get_action("CopyAllRight").set_sensitive(pane+1
< self.num_panes)
+        self.actiongroup.get_action("CopyAllLeft").set_sensitive(pane
> 0 and self.textview[pane-1].get_editable())
+        self.actiongroup.get_action("CopyAllRight").set_sensitive(pane+1
< self.num_panes and self.textview[pane+1].get_editable())

These changes are unrelated to the other functionality. Can you please
split them out into a separate patch and fix the operator spacing?

The rest of the changes look good to me, except that (as above) I'm
unclear on the usefulness of merge_2_files() currently.