When detecting a big file the method was changed to quick compare. So if you have more files which are text files and smaller, they all have been compared by quick content.
This patch is changeing the method only for the single file.
- in FolderCmp::prepAndCompareTwoFiles() we read compare method from compare context.
- compare method is changed only to local variable of FolderCmp::prepAndCompareTwoFiles()
- when we call FolderCmp::prepAndCompareTwoFiles() for next files we read the compare method again from compare context
I fail to see how the changed local variable could affect value in compare context. So this really should be per-file setting.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
corret,but this context has been changed!
Test with a foldercomper and some files. Test with debugger
put a breakpoint after
int nCompMethod = pCtxt->m_nCompMethod;
change this value maually to 1 (quick content) and see what happend.
With next call this value remains 1 as pCtxt has been changed by
*m_pCtx->GetCompareOptions(CMP_QUICK_CONTENT) in qick content.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
> I think this ok for both.
No it is not. I don't want hacks like that while we can have real fixes. Hacks only cause that nobody bothers to fix the real bugs. But since we can't add real fix for 2.12. branch the hack is ok there.
I think we need to refactor options handling a bit. CompareContext idea has been to contain per-compare data. Compare options etc. Currently we keep only current options. But as this bug shows it can lead to wrong data to hold. I think we need to keep array of compare options in CompareContext and just pick correct set for each file. I think that was my original plan but then run out of time and ended up doing it a wrong way.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
>I think we need to keep array of compare options in CompareContext
>and just pick correct set for each file.
No, that's not ok.
As now we start with a setting, that must be ok for one complete folder run.
So by F5-key we need to get new setting form options, than it's ok.
But what happend we have some running instances of WM. Do we want to have different setting there?. I think for that we should start new item in forum.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Different instances have nothing to do with this. Do not create problems we don't have. Even different compare windows have nothing to do with this. CompareContext is created for every compare and holds compare-specific data. Compare context may perfectly well hold several instances of compare options. Only downside is wasting few bytes of memory.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Yep, but this temporarily compare method switch is only member variable which is not stored to registry. We store to registry only options that should be remembered between instances.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
So now I think we should switch to forum with the discussion. As we have more bugreports relatet to the options, settings and colors also. Will be a common threat.
Do you start, or myself?
Let's go on here with our bug only.
A better solution is allways the death of a running (good) one.
So aslong we have no better ues it for truck and bruch.
Keep this item open till we have an other better solution.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
An other possibility to solve that bug
GetCompareOptions():
is it a must to chamge to method there, even we just want to get the correct DiffutilsOptions or QuickCompareOptions
so just remove the line
m_nCompMethod = compareMethod;
would solve that also.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
GetCompareOptions() was not only getting the diffvalues also setting the compare method.
Setting must be done by a setmethod or done as now in CreateCompareOptions().
added new patch.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
This really has nothing to do with general options, or bugs in there. Can we please focus on this particular bug, not re-doing WinMerge once again. Damn, it this is about one member variable updated and not reset back.
I still think (not having much time to to think about this) the best solution is to create separate instances of settings for each compare method. That means we create settings data once when compare begins and we don't need to re-create it when changing method. So it is faster that way even when it means it takes few bytes more memory.
I'm not after quick tricks to fix this. I'm after best solutions we can have. There are million of bad ways to fix this and I'm not interested in them. And why I know which is good way? Because I've designed and implemented this code from beginning.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
CreateCompareOptions() is creating our settings and can set as the name says also the
comparemethod.
GetCompareOptions() has to return the needed options for the method.
>Damn, it this is about one member variable updated and not reset back.
yes, but so my opion, setting done in wrong function, thats all.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I see your point. But I fail to see that as other than quick fix. It does not solve the whole problem.
The problem is bigger and more complex. CDiffContext::m_nCompMethod is meant to be the *main* compare method. In practice the method selected in options. It is the master variable that determines how lots of things work during compare. So it was plain error from my side to forget its role and use with compare options. It has nothing to do with them directly. Master compare mode doesn't change as compare options might change during compare.
After really thinking about this for a while I decided the solution is we add new int m_currentCompareMethod to CDiffContext. It is the *current* effective compare method and may change with the compare options. m_currentCompareMethod can change per file but m_nCompMethod is constant during the whole compare.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Submitted patch
#2803152 Fix bug that compare method was not restored to global value http://winmerge.org/patch/2803152
which is the correct fix for trunk.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Committed patch based your first patch to 2.12 branch:
Completed: At revision: 6831
> I don't see a sence there.
The current code has several weak points. First weakness is that code doesn't mode the reality that we have two compare methods in use. So code gets confused when the method change happens and there is no good way to handle it. Second weakness is that the compare method is public attribute which could be changed by anybody seeing the class. Which is just invitation for more bugs. Reality is main compare method does not change during the folder compare and code must model this.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
reset compare method to original
Any proof of this bug?
- in FolderCmp::prepAndCompareTwoFiles() we read compare method from compare context.
- compare method is changed only to local variable of FolderCmp::prepAndCompareTwoFiles()
- when we call FolderCmp::prepAndCompareTwoFiles() for next files we read the compare method again from compare context
I fail to see how the changed local variable could affect value in compare context. So this really should be per-file setting.
corret,but this context has been changed!
Test with a foldercomper and some files. Test with debugger
put a breakpoint after
int nCompMethod = pCtxt->m_nCompMethod;
change this value maually to 1 (quick content) and see what happend.
With next call this value remains 1 as pCtxt has been changed by
*m_pCtx->GetCompareOptions(CMP_QUICK_CONTENT) in qick content.
Oh! Now I see it. Thanks for explanation.
This is very important bug now to fix as it really explains some of those weird-looking bugs with weird results when comparing large/big files.
I think your fix is OK for the 2.12 branch. But for trunk we need a better solution. We must fix the way changing compare options works.
I think this ok for both.
What is to be done, is how and at what time we pass new diff options to pCtxt.
> I think this ok for both.
No it is not. I don't want hacks like that while we can have real fixes. Hacks only cause that nobody bothers to fix the real bugs. But since we can't add real fix for 2.12. branch the hack is ok there.
I think we need to refactor options handling a bit. CompareContext idea has been to contain per-compare data. Compare options etc. Currently we keep only current options. But as this bug shows it can lead to wrong data to hold. I think we need to keep array of compare options in CompareContext and just pick correct set for each file. I think that was my original plan but then run out of time and ended up doing it a wrong way.
>I think we need to keep array of compare options in CompareContext
>and just pick correct set for each file.
No, that's not ok.
As now we start with a setting, that must be ok for one complete folder run.
So by F5-key we need to get new setting form options, than it's ok.
But what happend we have some running instances of WM. Do we want to have different setting there?. I think for that we should start new item in forum.
Different instances have nothing to do with this. Do not create problems we don't have. Even different compare windows have nothing to do with this. CompareContext is created for every compare and holds compare-specific data. Compare context may perfectly well hold several instances of compare options. Only downside is wasting few bytes of memory.
No that's not what I mean.
When we change options they are stored to registry, isn't it.
Yep, but this temporarily compare method switch is only member variable which is not stored to registry. We store to registry only options that should be remembered between instances.
So now I think we should switch to forum with the discussion. As we have more bugreports relatet to the options, settings and colors also. Will be a common threat.
Do you start, or myself?
Let's go on here with our bug only.
A better solution is allways the death of a running (good) one.
So aslong we have no better ues it for truck and bruch.
Keep this item open till we have an other better solution.
An other possibility to solve that bug
GetCompareOptions():
is it a must to chamge to method there, even we just want to get the correct DiffutilsOptions or QuickCompareOptions
so just remove the line
m_nCompMethod = compareMethod;
would solve that also.
change in diffcontex
GetCompareOptions() was not only getting the diffvalues also setting the compare method.
Setting must be done by a setmethod or done as now in CreateCompareOptions().
added new patch.
This really has nothing to do with general options, or bugs in there. Can we please focus on this particular bug, not re-doing WinMerge once again. Damn, it this is about one member variable updated and not reset back.
I still think (not having much time to to think about this) the best solution is to create separate instances of settings for each compare method. That means we create settings data once when compare begins and we don't need to re-create it when changing method. So it is faster that way even when it means it takes few bytes more memory.
I'm not after quick tricks to fix this. I'm after best solutions we can have. There are million of bad ways to fix this and I'm not interested in them. And why I know which is good way? Because I've designed and implemented this code from beginning.
CreateCompareOptions() is creating our settings and can set as the name says also the
comparemethod.
GetCompareOptions() has to return the needed options for the method.
>Damn, it this is about one member variable updated and not reset back.
yes, but so my opion, setting done in wrong function, thats all.
I see your point. But I fail to see that as other than quick fix. It does not solve the whole problem.
The problem is bigger and more complex. CDiffContext::m_nCompMethod is meant to be the *main* compare method. In practice the method selected in options. It is the master variable that determines how lots of things work during compare. So it was plain error from my side to forget its role and use with compare options. It has nothing to do with them directly. Master compare mode doesn't change as compare options might change during compare.
After really thinking about this for a while I decided the solution is we add new int m_currentCompareMethod to CDiffContext. It is the *current* effective compare method and may change with the compare options. m_currentCompareMethod can change per file but m_nCompMethod is constant during the whole compare.
I don't see a sence there. We have a temp variable in foldercomper
prepAndCompareTwoFiles(), nCompMethod
isn't that enough?
what do you mean with 'per file'?
Submitted patch
#2803152 Fix bug that compare method was not restored to global value
http://winmerge.org/patch/2803152
which is the correct fix for trunk.
Committed patch based your first patch to 2.12 branch:
Completed: At revision: 6831
> I don't see a sence there.
The current code has several weak points. First weakness is that code doesn't mode the reality that we have two compare methods in use. So code gets confused when the method change happens and there is no good way to handle it. Second weakness is that the compare method is public attribute which could be changed by anybody seeing the class. Which is just invitation for more bugs. Reality is main compare method does not change during the folder compare and code must model this.