Menu

#2813 stringdiffcolor.

Trunk
closed-accepted
nobody
GUI (476)
5
2009-03-25
2009-02-25
Matthias
No

this patch improves coloring in a line while editing in docview.

See discussion in forum deleloper.

Discussion

1 2 > >> (Page 1 of 2)
  • Matthias

    Matthias - 2009-02-25

    see discuss in Forum: Coloring stringdifference

     
  • Kimmo Varis

    Kimmo Varis - 2009-02-26

    - return ch==',' || ch==';' || ch==':' || ch=='.';
    + return (_tcschr(_T(".,;:()[]{}!@#\"$%^&*~+-=<>\'/\\|"), ch)!=0);
    Ok, so there *were* hardcoding for punctuation chars. Not good.

    It appears Perry added that line basically in his first implementation. Hmm.

    Ok. Submit a separate patch which changes only *that* line and I'll commit it for 2.12 branch too.

    - for (int i = 0; i < pDiffs->size(); i++)
    + for (UINT i = 0; i < pDiffs->size(); i++)
    These silent the warning, but these are not the correct way. std::vector::size() returns size_type, not UINT.

     
  • Matthias

    Matthias - 2009-02-26

    hope I understand correct

    if I put size_type instead of unint I get compilererror?

     
  • Kimmo Varis

    Kimmo Varis - 2009-02-26

    Don't care about those warnings (not related to this patch) now.

    The correct fix would be to use std::vector<"type">::size_type, e.g.
    for (std::vector<wdiff*>::size_type i=0; i<m_wdiffs.size(); ++i)

    But please different patch. Don't try to fix everything in one patch. I keep saying this...

     
  • Matthias

    Matthias - 2009-02-26

    so now I think that's what you expect.

     
  • Kimmo Varis

    Kimmo Varis - 2009-02-27

    Can we just get the important changes in one patch first? Forget the warning fixes until the fixes are committed.

    Submit a patch that contains your fixes for highlighting, like you said in the original description. Do not include:
    - changes to punctuation chars
    - warning fixes (adding (u)int casts)

    Once that patch is committed we can think about other changes.

    And please say in the comment when you attach a file, SF.net doesn't indicate in any way that new file is attached and I very easily miss the files.

     
  • Matthias

    Matthias - 2009-02-27

    now here is my next version. I created the patch only with what is realy relatet to my changes.
    I had to add an new item to struc of word, cause we need this info of break for coloring.

    Now we have also a new posibility to color inserts diff to changes, but that will be a other patch/task.
    change the value m_matchblock (true) to false to compare hexfiles! much better result.

     
  • Matthias

    Matthias - 2009-02-27

    I thinks this will be a nice feature.
    Inserted or deletet chars/words will be colored differnd to changed one.

     
  • Kimmo Varis

    Kimmo Varis - 2009-02-27

    We really need more unit tests first. As I've said before I don't want to switch current bugs to some other bugs. I'm currently adding some basic tests...

     
  • Kimmo Varis

    Kimmo Varis - 2009-03-02

    I just submitted a patch for adding new option for wordbreak characters:
    #2655563 Add option to set wordbreak characters for highlight
    http://winmerge.org/patch/2655563

     
  • Kimmo Varis

    Kimmo Varis - 2009-03-03

    I just noticed there is a patch for totally new feature?? (ViewColorV1.patch)

    Can you submit it as a totally new tracker item? It is totally separate new feature from this item.

     
  • Kimmo Varis

    Kimmo Varis - 2009-03-03

    I committed the -patch to SVN trunk:
    Completed: At revision: 6523

    But now many unit tests are broken. Looks like the highlight end is not properly set. Eg in test StringDiffsTest / WordBreak2Words1:
    sd_ComputeWordDiffs("aBcde fghij", "abcde fghij", true, 0, 0, false, &diffs);
    the second diff end is at index 10, not 4 like it was before! So we highlight word that is not different!

    Many similar breakages. Do you have a quick fix in mind?

     
  • Matthias

    Matthias - 2009-03-03

    oh, I just wanted to show what is possible. If you like this feature to go one, I move that patch to a new item.
    Please just see how you find it needed or not.

    With my coloring 'm trying somethings different now. Sitll I have the problem to make the first letter. WM has also problem in case of more than 20 % diff in a line. Also if it is close. I think i can solve. Will take some more days testing. With unittest you can't see what I'm looking for.

     
  • Kimmo Varis

    Kimmo Varis - 2009-03-03

    I reverted the patch from SVN trunk because of regressions in unit tests:
    Completed: At revision: 6525

    > With unittest you can't see what I'm looking for.
    Actually unit tests just saved me doing a buggy WinMerge release. Testing sd_ComputeWordDiffs() is easy and adding new unit tests is easy. So next time all unit tests must pass before I commit a string diff patch.

    > If you like this feature to go one, I move that
    > patch to a new item.
    I am interested about it. Of course we need to test it more etc, but it looks interesting.

     
  • Matthias

    Matthias - 2009-03-06

    moved the colorering in view to ID:2669526

    new patch for this item will come soon.

     
  • Matthias

    Matthias - 2009-03-07

    I had to correct some functions, as the expected results are wrong, even it has show ok with the actual software.

     
  • Kimmo Varis

    Kimmo Varis - 2009-03-07

    Wrong. You try to change test when you should fix the code. The current tests PASS current code. And they really are simple enough that the assumptions I've made about the results have

    - EXPECT_TRUE(diffs.size() == 0);
    + EXPECT_EQ(0, diffs.size());

    I think these changes are pretty pointless. But perhaps the test is easier to understand with EXPECT_EQ so ok for me. So I'll commit them as a cleanup.

    > @@ -139,7 +140,8 @@
    > {
    > std::vector<wdiff*> diffs;
    > sd_ComputeWordDiffs(" abcde abcde", "abcde abcde", false, 1, 0, false, &diffs);
    - EXPECT_TRUE(diffs.size() == 0);
    + EXPECT_EQ(1, diffs.size());

    So you broke whitespace ignore. I think the test is valid, there should not be differences in the strings if whitespace is ignored. Even if there were difference, you need to add tests to test the difference is correct.

    There are other similarly "fixed" tests.

    > @@ -179,7 +182,7 @@
    > {
    > std::vector<wdiff*> diffs;
    > sd_ComputeWordDiffs("abcde abcde", "abcdeabcde", true, 2, 0, false, &diffs);
    > - EXPECT_TRUE(diffs.size() == 1);
    > + EXPECT_EQ(3, diffs.size());

    What?! Removing a space produces three diffs? That must not be right!

    In general, if you want to change a test then you really need to tell why the current test is broken. I really thought about those assumptions and I think they are mostly solid ones. There are some cases where I think we should return two diffs and I added comments to those cases.

     
  • Matthias

    Matthias - 2009-03-07

    1.)yes, you have understand correct, it's eaysier to see the problem.
    2.)if it means 'whitespace ignore' WM ignores all difference, even they are not there, than I'm wrong. I'm surpriesed, as noboody said somemthing diff in the forum on some bug reports.
    Now I read again manual, shows I'm realy wrong. Ok, I will change.
    3.) it's relatet to me. But still I want same and better result.
    So I detect single words, also spaces as a diff block. That's make it much easier to compare.
    An exampel, if we have a line with 10 words, and every all second is diff, we get 5 diff.
    If second and thirt are diff, we now get two instead of one. That's the main change to original source.

     
  • Matthias

    Matthias - 2009-03-08

    So I correct the broke whitespace ignore with new Stringdiff_test2.patch.
    I also added some more test to show where WM actually breaks.
    Test it, and let's discuss about the result.

     
  • Kimmo Varis

    Kimmo Varis - 2009-03-08

    I committed the EXPECT_TRUE --> EXPECT_EQ cleanups.

     
  • Kimmo Varis

    Kimmo Varis - 2009-03-08

    I won't apply changes like this:

    - EXPECT_EQ(0, pDiff->start[0]);
    - EXPECT_EQ(0, pDiff->start[1]);
    - EXPECT_EQ(4, pDiff->end[0]);
    - EXPECT_EQ(4, pDiff->end[1]);
    + if (diffs.size() >=1 )
    + {
    + EXPECT_EQ(0, pDiff->start[0]);
    + EXPECT_EQ(4, pDiff->end[0]);
    + }

    You are removing checks that check both sides see the correct diff.

     
  • Matthias

    Matthias - 2009-03-08

    agree, i didn't want to remove that. But than correct like that only.
    EXPECT_EQ(2, diffs.size());
    wdiff *pDiff = diffs[0];
    if (diffs.size() >=1 )
    {
    EXPECT_EQ(0, pDiff->start[0]);
    EXPECT_EQ(0, pDiff->start[1]);
    EXPECT_EQ(4, pDiff->end[0]);
    EXPECT_EQ(4, pDiff->end[1]);
    }
    if (diffs.size() >=2 )
    {
    pDiff = diffs[1];
    EXPECT_EQ(6, pDiff->start[0]);
    EXPECT_EQ(6, pDiff->start[1]);
    EXPECT_EQ(10, pDiff->end[0]);
    EXPECT_EQ(10, pDiff->end[1]);
    }

    So incase I have less diff as exspected, the prog will not break.

     
  • Kimmo Varis

    Kimmo Varis - 2009-03-08
    • milestone: 438015 --> Trunk
     
  • Kimmo Varis

    Kimmo Varis - 2009-03-08

    > So incase I have less diff as exspected, the prog will not break.
    Yeah, we can make tests handle errors better. But we should aim getting all tests pass. I'll commit valid parts as cleanups.

     
  • Kimmo Varis

    Kimmo Varis - 2009-03-08

    Cleanups committed. Now can you submit a patch with *changes* to the test results. And please explain why results need changing.

     
1 2 > >> (Page 1 of 2)

Log in to post a comment.