GNOME Bugzilla – Bug 680569
[PATCH] Hide empty folders (filter on dir state) + 2 minor bugfixes
Last modified: 2014-05-17 20:57:28 UTC
Created attachment 219610 [details] [review] filter dirs on state I have been using Meld for several months now and I really like it, but there has always been one thing that bugged me. When comparing 2 directories and unselecting Same: all the sub-dirs of the 2 roots are ALWAYS shown in the tree, even if they don't have any New/Modified files. As a result, the tree may have many empty folders (tree-branches), albeit collapsed. This can be very annoying, especially when comparing large directory trees with few differences. The first attached patch "1-filter_dirs_on_state.patch" makes it possible to hide empty folders, by adding filtering on state for directories, which acts the same way as for files. A dir is shown in the tree if one of the following is true: (1) it has files/folders in its sub-tree that match the state filters (2) it is empty (possibly by state filtering) and: - Same is active and the dir exists and is a (empty) dir in all roots - New is active and the dir doesn't exist in one of the other roots - Modified is active and the dir is not a dir but a file in another root The patch is surprisingly small: 4 minor (1 line) changes and 1 big(ger) change to the code. It has been tested, but only on a very limited scale. As this is my first try at programming in Python, it wouldn't surprise me if the code could be improved, especially regarding efficiency. Any suggestions are welcome. ======= The 2 other attached patches are minor bugfixes for: 2-fix_double_entries.patch: if A is a dir in root1, but a file in root2 then A gets added twice to the tree 3-fix_next.patch: use of global function next() (in dirdiff.py) which was introduced in Python 2.6; requirements for Meld 1.6.0 are Python >= 2.5 Note: before applying patch 2, you have to apply patch 1.
Created attachment 219611 [details] [review] fix double entries in the tree
Created attachment 219612 [details] [review] fix use of next()
I haven't had time to look at the first patch yet, so just commenting on the others. The double entries in the tree are intentional. The idea is that by aligning things in the tree, we're suggesting that they're comparable, but a file and a directory are clearly not comparable, and thus warrant different entries. There's also the more pragmatic aspect that having them on the same line would mean that we'd end up adding 'empty' children to a file, which just seems wrong. The use of next() is indeed wrong, thanks for catching that. However, current git has moved on to Python 2.6 (or 2.7, I haven't decided yet) so I've instead created a meld-1.6 branch for candidate stable branch fixes; the commit is on there.
Review of attachment 219610 [details] [review]: Firstly, let me say thanks for looking at this, and apologise for not getting around to this earlier. The patch is generally very nice, considering how unpleasant the solution has to be for our current tree model. I have a couple of general comments; more specific stuff is inline. While I know that the old code has a different style, any new code in Meld needs to be PEP8 formatted... this also applies to the often-broken 80 character line limit. Your commenting is very much appreciated, but I'd prefer if you could consolidate it a little bit, trying to explain the code block-by-block (where necessary) rather that appended to individual lines. If you're too busy to look at this stuff, let me know. I'm also happy to go ahead and make some of these changes, since they're generally pretty minor. ::: meld-1.6.0.orig/meld/dirdiff.py @@ +566,3 @@ todo.append(self.model.get_path(child)) + # depth first: put dirs alphabetically on stack, with the a's on top + if len(alldirs) > 0: todo[-len(alldirs):] = reversed(todo[-len(alldirs):]) Rather than this, could we just create a new temporary list and mess with that? It would seem less convoluted. Something like: child_dirs = [] for names in alldirs: entries = [os.path.join(r, n) for r, n in zip(roots, names)] child = self.model.add_entries(it, entries) differences |= self._update_item_state(child) child_dirs.append(self.model.get_path(child)) # depth first: put dirs alphabetically on stack, with the a's on top todo.extend(reversed(child_dirs)) @@ +574,3 @@ + all_present = False not in [ os.path.exists(f) for f in roots ] + all_dir = False not in [ os.path.isdir(f) for f in roots ] + if (not all_present) or (not all_dir) or (tree.STATE_NORMAL in self.state_filters): Doesn't isdir() imply exists()? Also, here and at several places below, you could make use of the any/all built ins. This could just be: all_dir = all(os.path.isdir(f) for f in roots) if not all_dir or tree.STATE_NORMAL in self.state_filters: @@ +576,3 @@ + if (not all_present) or (not all_dir) or (tree.STATE_NORMAL in self.state_filters): + self.model.add_empty(it) + if self.model.iter_parent(it) == None: Here, and several places below, comparisons against None should be done with 'is', not '=='. @@ +597,3 @@ + it = parent + # if parent has more children: state of these children is in state_filters; + # parent may not be removed in this case : stop while loop I don't think I properly understand this comment. Could you clarify? @@ +609,3 @@ + else: + # stop while loop: it == sibling files and sibling dirs come first in tree + break This could be simplified by pushing the isdir() check up into the loop conditional.
I just tested these patches on Meld 3.11.0 (git version) on OpenSuse 13.1 KDE 64 bit and worked fine. With improvements or not, it would be nice to have them merged some day...
+1 hello, this ("Hide empty folders (filter on dir state)") is the one thing that I find most annoying in Meld myself too... nice to see that a patch has been proposed 1.5 years ago.. great to hear it can still be applied.. what's holding back merging this in and releasing? - Is there anything I could do to help? PS: THANK YOU for Meld! ;-)
PS: http://stackoverflow.com/questions/21064163/what-do-i-have-to-install-to-resolve-could-not-find-any-typelib-for-gtksource-c ;-)
I've merged this patch to head, and then rewrote half of it using the patch logic as a basis. It looks to me like it works well in practice, but I haven't really stress-tested it.
*** Bug 476613 has been marked as a duplicate of this bug. ***
*** Bug 730249 has been marked as a duplicate of this bug. ***
So is this patch merged? Is there a commit number? What version is it included in?
Yes. de159dcde56950d99d333c1fe096c5d393bfa4b0 and subsequent commits. 3.11.0.