You really make it impossible for me to review these patches... There are clearly some undocumented dependencies for these patches, but there is no any description of that. Patches are overlapping in functionality (one adds calls to function other calls).
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I don' t think we want to cache this. This makes it a bit hard to follow who opens/closes files now. I think it must be simple - who uses the file opens and closes it. We don't want to keep files open longer than strictly necessary. There might be (and many times are) other applications wanting to open the files. And blocking their access is a bad thing.
I'm also wondering if we should expand DirItem / DiffFileInfo. Those already have most of the file info we will need. So why duplicate it to another struct.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
>You only want to switch from full content compare? Why not from quick contents?
will be done in next patch.
>We don't want to keep files open longer than strictly necessary.
All files are closed after comparing (m_diffFileData.Reset()).
>I'm also wondering if we should expand DirItem / DiffFileInfo
Hm, but that's now on you to take the decision.
See DiffFileInfo has much more infos as needed to handle files.
Should be better something subclass from FileLocation.
Also the complete filehandling (open, close, convert)should be done there.
Let be Filelocation part of file_data.
I just wanted to show how we can do, so same can be done for quickcontent also.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
> All files are closed after comparing (m_diffFileData.Reset()).
Yep. The DiffFileData is another bad structure from history. When we are re-thinking these classes we need to re-think DiffFileData too.
I need to think about these file structures. Collecting thread already reads lots of the file data so I think we should use that data we already have read. Using FileLocation we forget important data for some compare engines.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
>thread already reads lots of the file data so I think we should use that data we already
>have read.
That's what I want. Acutally we have the infos only in two diffent items.
FileLocation and file_data. So transfering the filehandling from file_data to FileLocation
or a inherit of that class would be fine. File_data used in FullContent can be used as now, temporary. So we can split the patches in small parts.
FileLocation must hold only datainfos about files and handling. To hold the content of lines inside this class is not good. Here the engines should take care in there own class.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The reason the file_data (and most of FolderCmp class) exists is to run translations (file content, not GUI) and plugins. Removing plugins means we can probably simplify that translation/encoding code quite a bit. And probably don't need that file_data at all. And don't need it for keeping file handles open.
I think we need to give full paths to files for compare engines. That way all engines have same kind of API. And it may even make sense to move translation/encoding code to inside compare engine.
But yes, this needs to be done in small steps. I'm not sure what would be the good first step.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
No, as we first have to know whitch kind of file we have, binary or text we need detection.
If we give only the path+filename, we have to reopen the file again. If that goes fast enough?
>Removing plugins
No, not now. If you have a replacement, ok.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
You think only couple of compare engines, not all of them. There content compare engines are not only compare engines. Engines comparing just file attributes don't need to ever open files. Currently we have to determine in FolderCmp (or elsewhere) if file should be opened and whatever preliminary work needs to be done before doing the compare.
It would be a lot easier to allow compare engine to decide what it needs to do. If and when it wants to open files. If it needs to read encoding or some other info.
If we handle all that inside compare engine we only need to open file just once like now. We can for example structure compare engines so that there is kind of "master" engine for different major types (content and attributes currently) and that master then uses slave engines (current compare engines).
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
So if I understand correct.
One class with engines (Full-Quick, Binary etc..)
One class for atributes (Time, size, etc..)
So what is the advance, we move are problems from foldercompare one level down only.
We still need plugin and filetransform. Still we need m_diffFileData to get the results for GUI. The class FolderCmp should be able todo that in a good way.
I have to think more in detail to get a real better structure.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
You limit your thinking too much by current code. The point is to find good solutions and then modify current code. Even rewrite all the current code if needed. In proper steps of course.
> We still need plugin and filetransform.
Not necessarily in current form.
Still we need m_diffFileData to get the results for GUI
> definitely not. GUI needs list if Diffitems or Diffranges. How we create and update those lists in compare code is totally invisible for the GUI.
> The class FolderCmp should be able todo that in a
> good way.
I really doubt it. I think FolderCmp's role should be to setup compare engines and compare options. We must be able to encapsulate compare code to compare engines. Then we can optimize inside engines how each engine works best. Not letting outside code (in FolderCmp and other classes) to limit compare code.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
new switch to binarycompare
You really make it impossible for me to review these patches... There are clearly some undocumented dependencies for these patches, but there is no any description of that. Patches are overlapping in functionality (one adds calls to function other calls).
sorry I forgot to tell you need patch
'This is a new binarycompare as discussed in forum. ID: 2805046'
But if you like it more I can combine this one and ID: 2805046' to one.
patch #2805036 GuessCodepageEncoding miss binary and UTF8
must be installed also
After detecting encoding (GuessCodepageEncoding) this patch will switch to new
BinaryCompare #2805046 new CompareBinaryFiles
new switch for BinaryCompare
modified FilelLocation
removed DIFF.H
modified Filelocation to hold the needed infos for compare.
+ int desc; /* File descriptor */
I don' t think we want to cache this. This makes it a bit hard to follow who opens/closes files now. I think it must be simple - who uses the file opens and closes it. We don't want to keep files open longer than strictly necessary. There might be (and many times are) other applications wanting to open the files. And blocking their access is a bad thing.
+ if (nCompMethod == CMP_CONTENT &&
+ (m_diffFileData.m_FileLocation[0].encoding.m_binary ||
+ m_diffFileData.m_FileLocation[1].encoding.m_binary))
+ {
+ nCompMethod = CMP_BINARY;
+ }
You only want to switch from full content compare? Why not from quick contents?
I'm also wondering if we should expand DirItem / DiffFileInfo. Those already have most of the file info we will need. So why duplicate it to another struct.
>You only want to switch from full content compare? Why not from quick contents?
will be done in next patch.
>We don't want to keep files open longer than strictly necessary.
All files are closed after comparing (m_diffFileData.Reset()).
>I'm also wondering if we should expand DirItem / DiffFileInfo
Hm, but that's now on you to take the decision.
See DiffFileInfo has much more infos as needed to handle files.
Should be better something subclass from FileLocation.
Also the complete filehandling (open, close, convert)should be done there.
Let be Filelocation part of file_data.
I just wanted to show how we can do, so same can be done for quickcontent also.
> All files are closed after comparing (m_diffFileData.Reset()).
Yep. The DiffFileData is another bad structure from history. When we are re-thinking these classes we need to re-think DiffFileData too.
I need to think about these file structures. Collecting thread already reads lots of the file data so I think we should use that data we already have read. Using FileLocation we forget important data for some compare engines.
>thread already reads lots of the file data so I think we should use that data we already
>have read.
That's what I want. Acutally we have the infos only in two diffent items.
FileLocation and file_data. So transfering the filehandling from file_data to FileLocation
or a inherit of that class would be fine. File_data used in FullContent can be used as now, temporary. So we can split the patches in small parts.
FileLocation must hold only datainfos about files and handling. To hold the content of lines inside this class is not good. Here the engines should take care in there own class.
reduced to min infos needed
how about this solution.
only passing the File.desc on an array for BinaryCompare.
That can be also enough for QickContent.
The reason the file_data (and most of FolderCmp class) exists is to run translations (file content, not GUI) and plugins. Removing plugins means we can probably simplify that translation/encoding code quite a bit. And probably don't need that file_data at all. And don't need it for keeping file handles open.
I think we need to give full paths to files for compare engines. That way all engines have same kind of API. And it may even make sense to move translation/encoding code to inside compare engine.
But yes, this needs to be done in small steps. I'm not sure what would be the good first step.
No, as we first have to know whitch kind of file we have, binary or text we need detection.
If we give only the path+filename, we have to reopen the file again. If that goes fast enough?
>Removing plugins
No, not now. If you have a replacement, ok.
You think only couple of compare engines, not all of them. There content compare engines are not only compare engines. Engines comparing just file attributes don't need to ever open files. Currently we have to determine in FolderCmp (or elsewhere) if file should be opened and whatever preliminary work needs to be done before doing the compare.
It would be a lot easier to allow compare engine to decide what it needs to do. If and when it wants to open files. If it needs to read encoding or some other info.
If we handle all that inside compare engine we only need to open file just once like now. We can for example structure compare engines so that there is kind of "master" engine for different major types (content and attributes currently) and that master then uses slave engines (current compare engines).
So if I understand correct.
One class with engines (Full-Quick, Binary etc..)
One class for atributes (Time, size, etc..)
So what is the advance, we move are problems from foldercompare one level down only.
We still need plugin and filetransform. Still we need m_diffFileData to get the results for GUI. The class FolderCmp should be able todo that in a good way.
I have to think more in detail to get a real better structure.
You limit your thinking too much by current code. The point is to find good solutions and then modify current code. Even rewrite all the current code if needed. In proper steps of course.
> We still need plugin and filetransform.
Not necessarily in current form.
Still we need m_diffFileData to get the results for GUI
> definitely not. GUI needs list if Diffitems or Diffranges. How we create and update those lists in compare code is totally invisible for the GUI.
> The class FolderCmp should be able todo that in a
> good way.
I really doubt it. I think FolderCmp's role should be to setup compare engines and compare options. We must be able to encapsulate compare code to compare engines. Then we can optimize inside engines how each engine works best. Not letting outside code (in FolderCmp and other classes) to limit compare code.