I did some fixes and improvements to chm package, LHelp and chmmaker - https://github.com/serbod/lazhelp

I contacted with maintainer, Andrew Haines, but then he disappear.

There is too many changes in many files, and that can't be posted as simple diff patch. Also, there is some suggestions about package contents and file/dir locations:

1. FPC package 'chm' contain file fasthtmlparser.pas, that used in some non-CHM packages and projects. May be better move that file to some generic package - fcl-base, for example?

2. FPC package 'chm' contain LZX compression routines in some files
- paslzx.pas, paslzxcomp.pas, paslznonslide.pas, lzxcompressthread.pas. They not depends on other files in 'chm' and can be maintained as separate package.

3. I suggest to move LHelp project from lazarus/components/chmhelp/lhelp to lazarus/tools/lhelp, because LHelp is idependent external utility and not internal part of IDE or users projects.

4. Also, I suggest to move lazarus/doceditor to lazarus/tools/doceditor for the same reason

FPC package 'chm' contain LZX compression routines in some files

- paslzx.pas, paslzxcomp.pas, paslznonslide.pas, lzxcompressthread.pas

It makes sense to move them to new package, and rename files! to lzx_*.pas. I like such names with _.

No, I am absolutely against renaming units just because somebody likes the new name more. Renaming units always breaks existing code of somebody. This is disrespectful against our users.

I would be willing to look at your lhelp version which probably is much needed. But the problem is that you also change units which are part of fpc, and I neither can nor want to change anything related to fpc. Even if you are able to get the fpc-related patches into fpc it will take a long time until this is available to Lazarus - we all know how long it took to release v1.8.

Or should we try to duplicate the chm units of fpc in Lazarus for easier modification?

#1 -- I, personally, would keep it where it is and copy it as "LazFastHtmlParser" to LazUtils which would offer more flexibility. The capitalization of the entire html text, for example, should be something to be turned off optionally (I'd need this in fpspreadsheet for a sufficiently fast sax-like access to xlsx files), and there's no chance to achieve this without forcing users to work with fpc trunk.

#2 -- Should be discussed on the fpc mailing list.

#3 and #4 do make sense.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lazarus-ide.org/pipermail/lazarus/attachments/20171208/2c419d6c/attachment.html>

Werner Pamler wrote

I am absolutely against renaming units

When user uses new package lzx, he adds new req in project. Then he also can change unit names in "uses".

Sergey Bodrov wrote

There is too many changes in many files, and that can't be posted as simple diff patch.

Then layer them. Make patches for the most essential bits, and then reallign your sources.

1. FPC package 'chm' contain file fasthtmlparser.pas, that used in some non-CHM packages and projects. May be better move that file to some generic package - fcl-base, for example?

Afaik this was meant as a copy of that parser dedicated for chm that could evolve with the package. So if you need it more general, whip up a suitable general version.

2. FPC package 'chm' contain LZX compression routines in some files - paslzx.pas, paslzxcomp.pas, paslznonslide.pas, lzxcompressthread.pas. They not depends on other files in 'chm' and can be maintained as separate package.

What does that make possible that currently is not possible? Do we really have to spin each unit into a separate package? I don't see the point.

Note also that iirc CHM lzx and e.g. CAB lzx vary in some ways. (headers?)

But even bypassing that, what is the problem of having to instal 10 extra units (150kb) to make a CAB decompressor?

Sergey Bodrov wrote

There is too many changes in many files, and that can't be posted as simple diff patch.

Use the Git clone (mirror) of the Lazarus (svn) repository. Git allows you to make local branch and local commits - invaluable in today's development (personal opinion, but agreed by many). You can then send pull requests - many Lazarus SVN developers seem to be willing to accept that, but confirm with the maintainers of that section of the project first. Alternative, you can ask git to generate a patch set by simply giving the starting and ending commit SHA1 values. It will generate many patch files in the order that they need to be applied.

Lazarus Git mirror: https://github.com/graemeg/lazarus.git

To clone from the command line:

git clone https://github.com/graemeg/lazarus.git

The above command only needs to be done once.

    git checkout -b my_features_branch
    // make your commits here

The above creates your own local branch were you place your local commits. Good advice - commit often!

git format-patch -o /tmp/ origin/master..HEAD

The above will generate a patch set and output all the *.patch files into the /tmp/ directory.

Regards,
Graeme

yada,yada,yada git plug

I assume you haven't read the message at all, since he did all that at github, but ran code formatters over it. So that makes diff, any diff a problem.

And pull requests also don't solve that.

Marco van de Voort wrote

yada,yada,yada git plug

Oh just fuck off Marco! Not everything I say is meant to piss you off or even add me to your email ignore list and be
done with it.

Regards,
Graeme

Or,... you could be a bit more selective to what you reply to.

I cloned your repo on my HD, and went through it step by step while always trying to build lhelp, I could remove everything from the folder chm except for these units: chmfifimain, chmreader, chmsitemap, chmtypes (and chmobjinstance.inc). I think it is possible to extract the essential changes from these units to get at least the new reading functionality. I can try this - but: before beginning I'd like to get positive feedback from fpc devs that this will be merged (I have some old patches hanging around there, and it is not always motivating to submit fpc patches).

As for the unit lcid_conv: This fits into Lazarus' LazUtils, and should go there independently of how this story ends here. I have commit rights to Lazarus and can put it there. Any objections?

Werner Pamler wrote

I think it is possible to extract the essential changes from these units to get at least the new reading functionality. I can try this [,.,]

And Sergey, you should explain which features are new? What do I have to do to see the new features in lhelp?

Sergey, I loaded a chm having codepage iso-8859-1 into your modified lhelp. It crashes in TIpChmDataProvider.DetectHtmlCodepage when trying to convert the string 88591 to a Word. Also, this codepage (as well as the other iso's) are not considered in lclid_conv.

Werner Pamler wrote

I cloned your repo on my HD, and went through it step by step while always trying to build lhelp, I could remove everything from the folder chm except for these units: chmfifimain, chmreader, chmsitemap, chmtypes (and chmobjinstance.inc).

I walked through chmreader, and found one minor fix, a comparison of a filename parameter with an extension should be lowercase. Fixed in r37714

And chmfifimain seems to have a memory leak in "destructor TChmSearchWriter.Destroy;" which does not call "inherited" -- see attached patch.
-------------- next part --------------
Index: chm/src/chmfiftimain.pas
=================================================================== --- chm/src/chmfiftimain.pas (revision 37715)
+++ chm/src/chmfiftimain.pas (working copy)
@@ -392,6 +392,7 @@

 begin
  freeandnil(FActiveLeafNode);
+ inherited;
 end;

Done.

2017-12-11 21:57 GMT+03:00 Werner Pamler via Lazarus <

There is no special new reading functionality, only bugfixes and code improvements. I spend many hours just to understand how it works and how it must work properly. It's not trivial without proper documentation.

I don't know, where is better place for such file. Locale ID is MS Windows specific, so maybe it better place to some windows - related unit or LazUtils?

Sergey, I loaded a chm having codepage iso-8859-1 into your modified lhelp. It crashes in TIpChmDataProvider.DetectHtmlCodepage when trying to convert the string 88591 to a Word. Also, this codepage (as well as the other iso's) are not considered in lclid_conv.

iso-8859-1 encoding have not relation to MS Locale ID. It HTML-specific, and must handled by HTML viewer. I do dirty hack with HTML codepage coversion just because don't ready to make fixes in TIpHtmlPanel.

2017-12-11 21:34 GMT+03:00 Werner Pamler via Lazarus <

: And Sergey, you should explain which features are new? What do I have to do to see the new features in lhelp?

- 'Copy raw souce' in page context menu.
- full-text search works properly
- RPC calls from Lazarus handled instantly
- codepage conversions (not anywhere, it still need testing)
- maybe some features related to multiple opened CHM files now works - global topics list, context search

many missed features from http://wiki.freepascal.org/chm related to HTML Viewer.

Sergey Bodrov wrote

 iso-8859-1 encoding have not relation to MS Locale ID. It HTML-specific, and must handled by HTML viewer. I do dirty hack with HTML codepage coversion just because don't ready to make fixes in TIpHtmlPanel.

But this specification occurs in the file and is not handled by your code. But anyway: If I understand you correctly you say that this part of your fixes is a hack around a IpHtmlPanel issue. Then you should know that I fixed code page conversion for the HtmlPanel some weeks ago in trunk, and just a minute ago I added another fix for the codepages which are not specified in the way LConvEncoding expects them ("windows-1252" instead of "cp-1252"). With these modifications I was able to see the German umlauts in the chm files I found on my HD correctly. Does this mean that your LCLID-related changes are obsolete? Please test. -------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lazarus-ide.org/pipermail/lazarus/attachments/20171212/03c1071c/attachment.html>

Sergey Bodrov wrote

I do dirty hack with HTML codepage coversion just because don't ready to make fixes in TIpHtmlPanel.

This is the wrong attitude. Please don't do dirty hacks but make fixes in other components as well. Or ask somebody to help you with it (Werner). The purpose of a patch is to make source code quality better and not worse.

Ondrej

Why not to use TEncoding provided with FPC 3?

Ondrej

2017-12-12 12:24 GMT+03:00 Werner Pamler via Lazarus <

Well, then my codepage and conversion procedure for HTML contents not needed anymore.
But LocaleID-based codepage conversion is still needed for topic names (Table of Contents and Index) and, maybe, for full-text search.

BTW, in ipcss.inc TCSSProps.ReadCommands() lack of Args.Count checking for 'border-width', 'border-color' and 'border-style' cause Args[0] out-of-range errors.
Instead of tons checks for Args.Count, I propose single local function, that returns string from Args by index, or empty string if there is no such index.

Really? It looks fine for me. I even removed some more checks for Args.Count > 0 when accessing Args[0] - this code is never reached because it happens inside an Args loop which is never entered if Args.Count = 0. What did you do to see that there is a problem?

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lazarus-ide.org/pipermail/lazarus/attachments/20171212/3e2810f9/attachment.html>

2017-12-12 16:32 GMT+03:00 Werner Pamler via Lazarus <

Maybe, you confused with ACommands loop? There is no loops for Args.

Sergey Bodrov wrote

Maybe, you confused with ACommands loop? There is no loops for Args.

Ah, of course. Thanks. I'll fix it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lazarus-ide.org/pipermail/lazarus/attachments/20171212/05201ac5/attachment-0001.html>

Sergey Bodrov wrote

- full-text search works properly

Can you explain where full-text search does not work properly? I need an example to work with, give detailed instructions how to reproduce this error.

I did not look into this feature in detail yet: Should full-text search work for any chm file, or must a full-text index be prepared when the file is written? In the latter case, can you provide a sample chm file suitable?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lazarus-ide.org/pipermail/lazarus/attachments/20171212/3f16d4a0/attachment.html>

2017-12-12 17:42 GMT+03:00 Werner Pamler via Lazarus <

It can find wrong pages, where no keywords in page body. For example - 'writeln' in rtl.chm
I don't sure, where exactly problems was, but defenitely something wrong was in WLC compressed integer reading funtion. Maybe also in WLC chunks reader.

I did not look into this feature in detail yet: Should full-text search work for any chm file, or must a full-text index be prepared when the file is written? In the latter case, can you provide a sample chm file suitable?

Full-text search works only when compiled into CHM. It also possible to create external full-text search index for CHM without embedded index, but this not yet implemented.

This is not the point. chm stores a Windows LCID to identify which codepage is used for index and table of contents within the chm. Therefore, LCID and codepage must be mapped to each other. This is what Sergey's unit lcid_conv does. TEncoding does not have anything to do with LCID.

I am no longer sure if it really fits into LazUtils because as I understand now the unit is needed also for writing - no it should be with the other chm units in (fpc)/packages/chm.

OK I see now: If I load rtl.chm into lhelp, go to page "Search" and type "Writeln" the current version finds a lot of pages, but the word "Writeln" is not contained in them. With your lhelp, on the other hand, it looks like all pages are correct and have the word "WriteLn" somewhere.

I compared your chmFiftiMain with the current FPC-trunk version and found the main difference in ReadWLCEntries. After copying your routine into the trunk unit I get the same search result in my LHelp as in your LHelp. Great!

If you don't mind I'll write a bug report for this part of your code. But a question before doing so: The original function ReadWLCEntries  returns a TChmWLCTopicArray, you put this into a var parameter and return a boolean value. But later you ignore the function result. Is there a special idea behind this?

Does your code write the full-text index? Or is it already contained in the current trunk version of chm?

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lazarus-ide.org/pipermail/lazarus/attachments/20171212/869f58f1/attachment.html>

On Dec 12, 2017 6:52 PM, "Werner Pamler" wrote:

If you don't mind I'll write a bug report for this part of your code. But a question before doing so: The original function ReadWLCEntries returns a TChmWLCTopicArray, you put this into a var parameter and return a boolean value. But later you ignore the function result. Is there a special idea behind this?

Strange way to make changes, but I don't mind.

Main idea is get sure, that functon worked properly and it's result is valid. And it make possible to do multiple searches with same resultset.

Does your code write the full-text index? Or is it already contained in the current trunk version of chm?

full-text index writer is already implemented in trunk, and seems to work fine. I just added some comments about how to properly use it and fix some potential bugs. Maybe also added option to chmmaker utility, don't remember..
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lazarus-ide.org/pipermail/lazarus/attachments/20171212/6a1bce07/attachment.html>

What is strange?

Why I disassemble your changes? In this case, you still would not have understood what this entire discussion is all about. Your sources will be rejected because the essential code is buried underneath tons of formatting changes. The developer who will review such a bug report will not simply take your sources and copy them over the official ones. He must understand what you changed, he must be able to study your sources for possible side-effects which the reporter often does not think of. With your code this is not possible. I think on the other hand, that it would be a pity if your work would not be considered. Therefore I joined this discussion. Now that I understood one of the issues you are fixing and was able to reproduce it I looked for differences between your code and the "official" one, and found that it would be sufficient to replace only a single procedure. With such a patch the reviewer knows what is fixed and how it is fixed. And my report
(https://bugs.freepascal.org/view.php?id=32814) gives him an easy way to verify that a bug exists and that the patch really fixes it.

Or is it strange that I write the bug report for you? I just wanted to help you. No problem if you write the next one yourself.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lazarus-ide.org/pipermail/lazarus/attachments/20171212/bab1c5cb/attachment.html>

No, but afaik an tencoding to lcid mapping would allow to convert?

I am no longer sure if it really fits into LazUtils because as I understand now the unit is needed also for writing - no it should be with the other chm units in (fpc)/packages/chm.

If it can be sufficiently clean (just the plain wrappings and maybe a converter helper), it could be stuffed in rtl-unicode. CHM is probably not the only thing that uses LCIDs.

2017-12-12 21:36 GMT+03:00 Werner Pamler via Lazarus <

http://wiki.freepascal.org/Creating_A_Patch describes two ways - make a small patch or send whole files with instructions, where they should be placed. So, I choose second way as more effective for massive changes. What's wrong?

I personally greteful, that you helps me. But I don't understand whole situation with code formatting, it looks like a some 'social bug' for me. If code formatting of existing code is prohibited, then must be some public warning in wiki.

Sergey Bodrov wrote

I personally greteful, that you helps me. But I don't understand whole situation with code formatting, it looks like a some 'social bug' for me. If code formatting of existing code is prohibited, then must be some public warning in wiki.

Looks like you really don't understand. I won't repeat what has been said before. Just one question: If you were the reviewer who added this patch to fcl and suddenly somebody finds out that this patch creates a crash somewhere else, how can you find the faulty line if there are thousands of irrelevant changes but 1 tiny incorrect "shr 1" somewhere?

If you really want code to be reformatted you must provide a separate patch which does not change anything in the logics of the code.

Sergey Bodrov wrote

> What is strange? http://wiki.freepascal.org/Creating_A_Patch describes two ways - make a small patch or send whole files with instructions

Hmm, it actuallya says: "new files should be sent as whole files, with instructions where they should be placed "

Changes to an existing file should not be set as a whole file, but as a patch.

Bart

Sergey Bodrov wrote

http://wiki.freepascal.org/Creating_A_Patch describes two ways - make a small patch or send whole files with instructions, where they should be placed. So, I choose second way as more effective for massive changes.

That is not true as Bart wrote. The page clearly says: ---
If you want to submit improvements to the FPC or Lazarus code, you need to submit a patch which developers can easily merge. Exceptions:
1. .po translation files should be sent as whole files
2. new files should be sent as whole files, with instructions where they should be placed
---

What's wrong?

That you refuse to understand things that are clearly explained in this mailing list and in wiki.
Is it a language issue? No, your English seems good. This starts to bother me, too.

Juha

2017-12-13 12:09 GMT+03:00 Werner Pamler via Lazarus <

I can't make review from single line of code or single function, that I don't know, how it works and how connected to another parts. If it obvious error, like missed inherited destructor, or known point of exception - then it easy. But when it some hidden bug, wrong behavior - then need some code preparing, that make wrong parts of code obvious or force some visible effect in runtime. Comments, naming, debug output, assertions. And only when code is perfectly looks and works, then it can be cleaned from redundant parts and treated as some blackbox.

I thinked, that sources of FPC compiler and RTL is that 'holy bible', where every line is critical and must be carefully checked. But CHM package is optional and not documented, it have many unused and debug lines. I even don't sure, that it actually part of FCL.

If you really want code to be reformatted you must provide a separate patch which does not change anything in the logics of the code.

It make a sense. But how to bring a patch with only code formatting? It must be single huge diff, or splitted for different blocks? And why file with massive changes can't be treated as new file?

2017-12-13 14:37 GMT+03:00 Juha Manninen via Lazarus <

:

That you refuse to understand things that are clearly explained in

this mailing list and in wiki.

Can massive changes in non-RTL packages can be treated as new files?

How to submit patches with massive formatting changes?

It's too painful to catch a bugs in messy code without any documentation. I thinking about just make a new namespace and forget about that problem for myself. But I want to understand, how developers of FPC made massive changes (as with Unicode handling) with such strict constraints.

No.

How to submit patches with massive formatting changes?

Like any other change: Just create a diff (i.e. patch).

It's too painful to catch a bugs in messy code without any documentation. I thinking about just make a new namespace and forget about that problem for myself. But I want to understand, how developers of FPC made massive changes (as with Unicode handling) with such strict constraints.

By using a version control system like svn or git and having test suites.

Mattias

No.

How to submit patches with massive formatting changes?

Hard. But if you for some odd reason feel the need to do all this reformatting etc, it doesn't matter as long as you keep your fixes separate (e.g. in a local version system, or just by keeping copies), and then insert them in the original, unreformated checkout and then make the patch.

It's too painful to catch a bugs in messy code without any documentation.

The problem is that messy is relative.

I thinking about just make a new namespace and forget about that problem for myself. But I want to understand, how developers of FPC made massive changes (as with Unicode handling) with such strict constraints.

It takes training. And one should start small, in keeping relative simple changes as small, reliable and unintrusive possible.

After a while it becomes a second nature.

Am 13.12.2017 13:05 schrieb "Sergey Bodrov via Lazarus" <

:

I thinked, that sources of FPC compiler and RTL is that 'holy bible', where every line is critical and must be carefully checked. But CHM package is optional and not documented, it have many unused and debug lines. I even don't sure, that it actually part of FCL.

No matter the project functional and formatting changes *always* have to be done separately as otherwise or is hard to see what changed for the functionality (e.g. "shr" instead of "shl" in completely reformatted code). The one who commits a patch to the development version must know what has been changed so that they can be sure that nothing will break, but that isn't possible if they can't see the forest due to all the trees.

Regards,
Sven
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lazarus-ide.org/pipermail/lazarus/attachments/20171213/7515087f/attachment-0001.html>

I added this feature to lhelp. Unlike in your solution fcl files are not modified which avoids the hassle that lhelp would not work any more with fpc 3.04 or older.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lazarus-ide.org/pipermail/lazarus/attachments/20171214/d66258ed/attachment.html>