GNOME Bugzilla – Bug 761028
Text filters should use same filter code in both folder and file comparisons
Last modified: 2016-03-14 21:09:13 UTC
See bug 758478 comment 12. We now combine the results from text filters instead of layering them in file comparisons, so we should do the same in folder comparison.
If it is all the same to you I would like to have a look at it in the next week or so, now that I know a little bit about the filtering.
Please do. Let me know if I can help out at all.
Created attachment 320495 [details] [review] Patch proposal Hi, this is my patch proposal. It is basically the same as we do in filediff. I tested it and it worked with my example. The only thing is that I still don't have a good overview of the code, that means that if there are other parts of the code that have to be adjusted as well, I did not find them. :-) Yours, David
Review of attachment 320495 [details] [review]: So I think just adjusting this section should probably be fine. Other comments below. ::: meld/dirdiff.py @@ +201,3 @@ + filter_ranges = misc.merge_intervals(filter_ranges) + for (start, end) in filter_ranges: + c = c[:start] + c[end:] It strikes me that this is basically a duplication of the first half of Filediff._filter_text(), but missing a few bits of logic. Could we just break out lines ~761-780 of filediff.py and put it into meld.misc so that we can use exactly the same code?
Created attachment 321298 [details] [review] New patch I see what you mean. I also looked again at my old patch proposal and realized that it was far from working fine. So I worked a little bit more on it. Please don't see this as the final solution, because I am not very sure about some parts of the patch and wanted to hear your opinion at this point before I continue. Although it seems to be working now. A little further explanation: I put everything that is the same for filediff and dirdiff into meld.misc. I also put the exception handling there, because we need to do the assertion in the "for filt in ..." loop to see which filter caused the exception for the warning message. (Btw, I apparently broke this with the new filediff filter mechanism.) Unfortunately this causes a little more changes: We have to work with filters instead of regular expressions in some places and instead of a function I did write a callable in meld.misc, to save the state of warned_bad_comparison. Also for dirdiff I am not sure if we have to assert that there are no newlines in the match, I think we don't have to. To have as less duplicate code as possible, and to have a nice apply_text_filters function with a clean interface, I transmit a cutter function as a parameter. So the caller can decide what's happening with the matches while the logic (i.e. the for loop and the reversing) stays in apply_text_filters. This prevents misstakes like I did in the last patch proposal: I forgot the reversed().
Created attachment 321303 [details] [review] Alternative Patch So, this is what it looks like in my head if we do without telling which filter caused the newline exception. It contains a little bit less changes.
Well in any case it would be nice to have a default cutter like "lambda start, end, txt : txt[:start] + txt[end:]" and to change the order of the parameters (I find it more natural to have txt as first parameter). But as I said, don't take this as a final patch proposal, but instead as the current state of work.
Review of attachment 321303 [details] [review]: So I can see why you've done it, but I'm not totally convinced by passing through the "cutter" function. To make the API simpler, I think we should do one of two things: * Have the new function in meld.misc *only* do the filter range selection and merging, and just pass back a list of merged ranges for dirdiff and filediff to handle themselves. Yes, this would involve a very small amount of code duplication, but it's not too bad. * Have the new function always do the cutting and pass back *both* the filtered text and the list of ranges. This way, filediff can just re-iterate over the ranges to do its highlighting. I'd probably prefer the first of these, but the second works as well. ::: meld/filediff.py @@ +781,2 @@ ) self.warned_bad_comparison = True So this text change is just a straight-up bug fix. If you could put that in a separate commit I'll pull that in immediately.
Created attachment 321884 [details] [review] Bugfix error message Here is the bugfix for the error message
Created attachment 321886 [details] [review] Overworked patch proposal To make the API simpler, I thought about a default cutter. I think this is the cleanest version. Please give it a second thought, I added the cleaned up patch. Of course, if you decide not to do it this way, I will leave out the cutter function.
(In reply to David Rabel from comment #9) > Here is the bugfix for the error message I've just pushed this now, thanks. (In reply to David Rabel from comment #10) > To make the API simpler, I thought about a default cutter. I think this is > the cleanest version. Please give it a second thought, I added the cleaned > up patch. I've only just got around to checking this out, sorry. In testing, I noticed that in the newly broken-out apply_text_filters(), we're no longer merging intervals. I assume this was just an oversight? Also, this doesn't apply cleanly to master because of a fix in 095b8331. If you could fix up those couple of things and resubmit, I'll happily push this.
Created attachment 323440 [details] [review] Patch proposal Thank you. Here is the patch proposal. I added the merging of the intervals that i forgot, also I added the fix you mentioned and made a new clean patch out of it from the current master. Yours, David
Excellent, cheers. I've just pushed your patch to master. Thanks very much again for the patches.