- 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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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...
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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
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.
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.
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
> 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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
see discuss in Forum: Coloring stringdifference
- 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.
hope I understand correct
if I put size_type instead of unint I get compilererror?
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...
so now I think that's what you expect.
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.
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.
I thinks this will be a nice feature.
Inserted or deletet chars/words will be colored differnd to changed one.
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...
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
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.
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?
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.
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.
moved the colorering in view to ID:2669526
new patch for this item will come soon.
I had to correct some functions, as the expected results are wrong, even it has show ok with the actual software.
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.
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.
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.
I committed the EXPECT_TRUE --> EXPECT_EQ cleanups.
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.
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.
> 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.
Cleanups committed. Now can you submit a patch with *changes* to the test results. And please explain why results need changing.