This still needs description. Just saying its been discussed somewhere without even giving a link to discussion is big no. Please try to understand we may need to refer and look to these items after few years. What is now clear is forgotten then. Always describe patches so that the description is useful even after few years. It is the whole point of using patch tracker. Yes it requires time to write good description. Yes, everybody does it.
This absolutely needs to be a new compare engine not just function in quick compare engine. So this is not what I wanted and described.
-
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
When I talk about compare engine I mean class like ByteCompare class. There is separate methods for setting options, running the compare and setting abortable interface. These are very important things to do. Aborting must be possible for large files. The binary compare engine must be able to run as independent and selectable engine, not just quick hack of some other engine.
It really requires some work to do. And I expect even testing takes few days. This will be very important feature in compare engines and it must be done with lots of care. And it must be tested too. I've put months of work into this current compare system so I really will make sure the new engine is done right. You can expect lots of comments.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
related to forum https://sourceforge.net/apps/phpbb/winmerge/viewtopic.php?f=6&t=115
this is a new patch, where I created new classes.
A basic FileCompare witch is used as common and inherit to BinaryCompare.
To have have two classes makes it easier to create new engines, as the common things are allready done in basic class (FileCompare).
you need to install also
#2805036 GuessCodepageEncoding miss binary and UTF8
and
#2805040 switch to new CompareBinaryFiles
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
New general file compare class is basically a good idea.
You cannot use diffutils internal structure in other compare engines:
+ file_data * m_inf; /**< Compared files data (for diffutils). */
This would make other engines dependent on diffutils internals which must not happen. Yes, I see quick compare uses it currently but we must fix that.
+#include "DIFF.H"
This is a BIG warning sign that something is wrong - it is diffutils header!
You have copied code and comments but haven't updated them to match new place. Comments in FileCompare class talk about quick compare etc.
FileTextStats really don't belong to base class - we only get text stats from one engine.
What is this CCompareOptions class?
+/**
+ * @brief Compare options used with compare -method.
+ * This class has some Basic Compare specifics in addition to general compare
+ * options.
+ */
Doesn't make any sense to me since it just seems to be binary file compare options with another name.
Also the name is just wrong - we don't prefix class names with "C" and now that prefix only differentiates it from other class name.
+ long tmp[2]; // pointer to file postion
long is NOT 64-bit in 32-bit Windows. You need 64-bit file positions to support large files.
+ _lseek(m_inf[i].desc, tmp[i], SEEK_SET);
You must use lseeki64 to support large files
+ // Did we finish both files?
+ // We set the text/binary status only for fully compared files. Only
+ // then the result is reliable.
The comment is wrong.
+ void SetAdditionalOptions(bool stopAfterFirstDiff);
Is there a situation when we don't want to stop after first different byte? I can' t think of any. Quick compare needs to read whole file to get reliable results since it handles text files.
+/**
+ * @brief check Abortable-interface.
You don't check interface but you check if user wants to stop the compare.
+/**
+ * @brief A quick compare -compare method implementation class.
+ * This compare method compares files in small blocks. Code assumes block size
+ * is in range of 32-bit int-type.
+ */
+class FileCompare
Update comment and perhaps describe THIS class...
+/**
+ * @brief Compare options used with Binary compare -method.
+ * This class has some Binary Compare specifics in addition to general compare
+ * options.
+ */
+class BinaryCompareOptions : public CCompareOptions
I don't see any specifics? Just CCompareOptions with new name.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
So we found another must be fixed issue - ByteCompare cannot use diffutils file_data struct. So we definitely need more refactoring before this patch can go in.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Also I'm really not sure about your base class for compare. It is base class for only couple of compare methods. What we do with classes that can't derive from your class? Have own base class for them? Or perhaps we need one base class just having filenames and Compare -function (CompareEngine). And then create class structure on that. It could be ContentCompare class deriving from CompareEngine. Then quick compare, byte compare and diffutils compare deriving from ContentCompare. Something like that.
And it would be another patch first creating the class structure and refactoring existing compare engines to use them. Then we can add this new engine using those classes. So we don't need to first add new engine and then refactor it in next patch.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
removed DIFF.H
modified Filelocation to hold the needed infos for compare.
Createing ContentCompare makes not much sence, as a lot of work is to be done in foldercompare.
The basic class should hold all common functios and flags,
even they are not used in some classes.
I create a new patch with modified FileLocation. With this content alL other method except full content (or some more refactoring) , will work perfect.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
> Createing ContentCompare makes not much sence, as a lot of work
> is to be done in foldercompare
> The basic class should hold all common functios and flags,
> even they are not used in some classes.
No. We don't want to have content compare functions for time/size compare engine.
We clearly have two different kind of compare engines - comparing content and comparing file attributes. These both clearly need their own base classes in good class design.
And attribute comparing will be very important in future, it is very fast and gives results lots of users need without wasting time with actual content. Lots of people only synchronize files with WinMerge and WinMerge is not good at that at the moment.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
your own words
>We clearly have two different kind of compare engines - comparing content
>and comparing file attributes. These both clearly need their own base
>classes in good class design.
nothing else:=)
But will take time to finish 3-4 month, sorry I'm out for some weeks.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
In FileCompare.h:
+ bool SetFilename(FileLocation *location, int fdesc[2]);
+ bool SetFilename(FileLocation *location, int fdesc[2], bool lLimited);
Why have return value if they always return true? Use b prefix for bool arguments. What does that limiting mean?
+ bool m_lLimited; //info compare of limited size
Use b as a prefix for booleans.
If you intent FileCompare class to not be used but only inherited then you should make it abstract class.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I'd done most changes.
for comparefiles I keeped a return value for later use.
boolblLimited means:
I case of method full content and filesize is over max we switch over to binary.
If now the result is diff, I want to give a message back to GUI that the compare is done in a diffrent method.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
fast byte to byte compare
This still needs description. Just saying its been discussed somewhere without even giving a link to discussion is big no. Please try to understand we may need to refer and look to these items after few years. What is now clear is forgotten then. Always describe patches so that the description is useful even after few years. It is the whole point of using patch tracker. Yes it requires time to write good description. Yes, everybody does it.
This absolutely needs to be a new compare engine not just function in quick compare engine. So this is not what I wanted and described.
-
When I talk about compare engine I mean class like ByteCompare class. There is separate methods for setting options, running the compare and setting abortable interface. These are very important things to do. Aborting must be possible for large files. The binary compare engine must be able to run as independent and selectable engine, not just quick hack of some other engine.
It really requires some work to do. And I expect even testing takes few days. This will be very important feature in compare engines and it must be done with lots of care. And it must be tested too. I've put months of work into this current compare system so I really will make sure the new engine is done right. You can expect lots of comments.
ok, I create a complete new class, will take some days to do.
the function you asked for are will realy work also, that's clear.
related to forum https://sourceforge.net/apps/phpbb/winmerge/viewtopic.php?f=6&t=115
this is a new patch, where I created new classes.
A basic FileCompare witch is used as common and inherit to BinaryCompare.
To have have two classes makes it easier to create new engines, as the common things are allready done in basic class (FileCompare).
you need to install also
#2805036 GuessCodepageEncoding miss binary and UTF8
and
#2805040 switch to new CompareBinaryFiles
create new classes
New general file compare class is basically a good idea.
You cannot use diffutils internal structure in other compare engines:
+ file_data * m_inf; /**< Compared files data (for diffutils). */
This would make other engines dependent on diffutils internals which must not happen. Yes, I see quick compare uses it currently but we must fix that.
+#include "DIFF.H"
This is a BIG warning sign that something is wrong - it is diffutils header!
You have copied code and comments but haven't updated them to match new place. Comments in FileCompare class talk about quick compare etc.
FileTextStats really don't belong to base class - we only get text stats from one engine.
What is this CCompareOptions class?
+/**
+ * @brief Compare options used with compare -method.
+ * This class has some Basic Compare specifics in addition to general compare
+ * options.
+ */
Doesn't make any sense to me since it just seems to be binary file compare options with another name.
Also the name is just wrong - we don't prefix class names with "C" and now that prefix only differentiates it from other class name.
+ long tmp[2]; // pointer to file postion
long is NOT 64-bit in 32-bit Windows. You need 64-bit file positions to support large files.
+ _lseek(m_inf[i].desc, tmp[i], SEEK_SET);
You must use lseeki64 to support large files
+ // Did we finish both files?
+ // We set the text/binary status only for fully compared files. Only
+ // then the result is reliable.
The comment is wrong.
+ void SetAdditionalOptions(bool stopAfterFirstDiff);
Is there a situation when we don't want to stop after first different byte? I can' t think of any. Quick compare needs to read whole file to get reliable results since it handles text files.
+ BinaryCompareOptions *m_pOptions; /**< Compare options for diffutils. */
Update comments.
+bool FileCompare::SetCompareOptions(const CompareOptions & options)
+{
+ return false;
+}
I don't understand this?
+/**
+ * @brief check Abortable-interface.
You don't check interface but you check if user wants to stop the compare.
+/**
+ * @brief A quick compare -compare method implementation class.
+ * This compare method compares files in small blocks. Code assumes block size
+ * is in range of 32-bit int-type.
+ */
+class FileCompare
Update comment and perhaps describe THIS class...
+ CCompareOptions *m_pOptions; /**< Compare options for diffutils. */
Update comment...
+ file_data * m_inf; /**< Compared files data (for diffutils). */
Update comment...
+/**
+ * @brief Compare options used with Binary compare -method.
+ * This class has some Binary Compare specifics in addition to general compare
+ * options.
+ */
+class BinaryCompareOptions : public CCompareOptions
I don't see any specifics? Just CCompareOptions with new name.
So we found another must be fixed issue - ByteCompare cannot use diffutils file_data struct. So we definitely need more refactoring before this patch can go in.
Also I'm really not sure about your base class for compare. It is base class for only couple of compare methods. What we do with classes that can't derive from your class? Have own base class for them? Or perhaps we need one base class just having filenames and Compare -function (CompareEngine). And then create class structure on that. It could be ContentCompare class deriving from CompareEngine. Then quick compare, byte compare and diffutils compare deriving from ContentCompare. Something like that.
And it would be another patch first creating the class structure and refactoring existing compare engines to use them. Then we can add this new engine using those classes. So we don't need to first add new engine and then refactor it in next patch.
removed unused parts
removed DIFF.H
modified Filelocation to hold the needed infos for compare.
Createing ContentCompare makes not much sence, as a lot of work is to be done in foldercompare.
The basic class should hold all common functios and flags,
even they are not used in some classes.
I create a new patch with modified FileLocation. With this content alL other method except full content (or some more refactoring) , will work perfect.
Better.
> Createing ContentCompare makes not much sence, as a lot of work
> is to be done in foldercompare
> The basic class should hold all common functios and flags,
> even they are not used in some classes.
No. We don't want to have content compare functions for time/size compare engine.
We clearly have two different kind of compare engines - comparing content and comparing file attributes. These both clearly need their own base classes in good class design.
And attribute comparing will be very important in future, it is very fast and gives results lots of users need without wasting time with actual content. Lots of people only synchronize files with WinMerge and WinMerge is not good at that at the moment.
oh, we mist us.
I mean foldercompare takes allready the decision what to do, so creating a new class for that makes not much sence.
I'm not meaning new class to replace folder compare. I'm talking about base class for compare engine classes.
So FileCompare can be our start. Let it grow.
useing filedescription in serperate array
to remove m_diffFileData takes a lot more todo.
So now i'm passing only file.desc to BinaryCompare.
> So FileCompare can be our start. Let it grow.
I don't understand this? Start of what? Grow? I don't want classes to grow, they are too fat already.
your own words
>We clearly have two different kind of compare engines - comparing content
>and comparing file attributes. These both clearly need their own base
>classes in good class design.
nothing else:=)
But will take time to finish 3-4 month, sorry I'm out for some weeks.
removed unused header
what else should be done here?
new basic class for foldercompare
please have a look to that patch.
Filecopmare will be a basic class
Binary the new binary compare.
The patch adds new code that is not called? Probably copies code from other classes?
I know this is copied code in BinaryCompare.cpp:
+ * @brief Compare two specified files, byte-by-byte
+ * @param [in] piAbortable Interface allowing to abort compare
+ * @return allways true
+/**
+ * @brief Default destructor.
+ */
+BinaryCompare::~BinaryCompare()
+{
+}
No need for empty destructor.
but I'd add comment saying it returns always true because byte-by-byte compare cannot fail.
In BinaryCompare.h:
+class CompareOptions;
+class CommonCompareOptions;
+class BinaryCompareOptions;
+class IAbortable;
+struct FileLocation;
+struct file_data;
Are all these forward declarations really needed?
In FileCompare.cpp:
+/**
+ * @brief Default destructor.
+ */
+FileCompare::~FileCompare()
+{
+}
No need for empty destructor.
In FileCompare.h:
+ bool SetFilename(FileLocation *location, int fdesc[2]);
+ bool SetFilename(FileLocation *location, int fdesc[2], bool lLimited);
Why have return value if they always return true? Use b prefix for bool arguments. What does that limiting mean?
+ bool m_lLimited; //info compare of limited size
Use b as a prefix for booleans.
If you intent FileCompare class to not be used but only inherited then you should make it abstract class.
I'd done most changes.
for comparefiles I keeped a return value for later use.
boolblLimited means:
I case of method full content and filesize is over max we switch over to binary.
If now the result is diff, I want to give a message back to GUI that the compare is done in a diffrent method.