My rich text control is still far from even semi-complete, but I think too much is done to start over, so I'm releasing it today. The number one reason of its incompleteness is that I put more emphasis on making things work exactly like in the original than on implementing more functions.
It's been neglected for weeks, and only recently I've fixed some old bugs and slowly started implementing the simplest parts of the RICHEDIT control API. I'm still not done with the basic stuff, like undo stack, so there is not much to hack on. On the other hand, it's likely I've made some bad decisions during design, and having someone experienced to look at that seemed to be a good idea to me.
Some things look really tricky to implement with the current architecture, like the printing API (it will likely be an ugly hack, but I expect the same from the original, as screen formatting and print formatting may be very different). Some things just need time. I will probably need others' help to do the RTF parser. It's just way too much work.
So, if anyone cares:
http://foltman.com/richtext-20050130.tar.gz
Compile using the recent-ish mingw (plus original SDK richedit header file).
Have fun, Krzysztof
Krzysztof Foltman wrote:
My rich text control is still far from even semi-complete, but I think too much is done to start over, so I'm releasing it today. The number one reason of its incompleteness is that I put more emphasis on making things work exactly like in the original than on implementing more functions.
Fantastic. This is good work. I haven't reviewed the code in depth, but I think the way forward is to submit an implementation of dlls/riched20 then make that work, and after it's debugged and worked out, rip out the old dlls/riched32 code and forward it to the new completed riched20 code.
Code review notes:
You've writen the code to deal with unicode as default, which is good, but you need to deal with both ASCII and unicode messages in the window procedure.
It might be better to use libwineunicode and kernel32 unicode string manipulation functions rather than msvcrt ones. eg lstrlenW, lstrcpyW, etc. instead of wcslen, wcscpy, etc. Avoid the TCHAR type in Wine code.
Similarly, you'll need to use "winbase.h" and friends instead of "windows.h".
From what I can see in the old riched32 code, the rtf parser looks quite good, so don't try rewrite that (which from my quick check you haven't). We can merge the parser into the riched20 code quite easily... it's just enters the text into the existing control using EM_SETSEL and some formatting messages.
I have a test I can send to you if you wish. Again, my preference is to get this into the Wine CVS sooner rather than later, so others can start helping improve it.
Mike
Mike McCormack wrote:
but I think the way forward is to submit an implementation of dlls/riched20 then make that work, and after it's debugged and worked out, rip out the old dlls/riched32 code and forward it to the new completed riched20 code.
I'd prefer to keep it as an app, not a DLL for a while. It just makes testing easier.
You've writen the code to deal with unicode as default, which is good, but you need to deal with both ASCII and unicode messages in the window procedure.
Sure. And I guess it should be done before making a DLL out of it.
It might be better to use libwineunicode and kernel32 unicode string manipulation functions rather than msvcrt ones. eg lstrlenW, lstrcpyW, etc. instead of wcslen, wcscpy, etc. Avoid the TCHAR type in Wine code.
Good point. I guess it would be nice to replace malloc/free with LocalAlloc/LocalFree, right ?
Similarly, you'll need to use "winbase.h" and friends instead of "windows.h".
Good idea. Done.
From what I can see in the old riched32 code, the rtf parser looks quite good, so don't try rewrite that
I'm all for reusing existing Wine code.
haven't). We can merge the parser into the riched20 code quite easily... it's just enters the text into the existing control using EM_SETSEL and some formatting messages.
LOL :) That's a smart idea :) I'm curious about interactions with undo, but we have no undo anyway (yet).
I have a test I can send to you if you wish.
You mean, another container application ? That would be cool, my container is getting better but it's still not very good.
Again, my preference is to get this into the Wine CVS sooner rather than later, so others can start helping improve it.
I'll see what can be done. As soon as there is a minimal text manipulation API (based on RichEdit API, not on function calls), I can separate the control into a DLL and move the app code in the test directory.
So far, the next version is available at:
http://foltman.com/richtext-20050130b.tar.gz
Main changes are:
- editor.c contains a list of which messages/notifications/styles work and which don't - incomplete read-only mode - replacement of MSVCRT wcs functions with Win32 lstr - a modification flag - a shy attempt at starting undo - a #define to choose between my editor and the original RICHEDIT. It's not what I want it to be, but it's better than nothing.
Krzysztof Foltman wrote:
Mike McCormack wrote:
It might be better to use libwineunicode and kernel32 unicode string manipulation functions rather than msvcrt ones. eg lstrlenW, lstrcpyW, etc. instead of wcslen, wcscpy, etc. Avoid the TCHAR type in Wine code.
Good point. I guess it would be nice to replace malloc/free with LocalAlloc/LocalFree, right ?
No, the LocalAlloc/LocalFree/GlobalAlloc/GlobalFree functions are relics from the old days of Win16. You should use HeapAlloc/HeapFree.
haven't). We can merge the parser into the riched20 code quite easily... it's just enters the text into the existing control using EM_SETSEL and some formatting messages.
LOL :) That's a smart idea :) I'm curious about interactions with undo, but we have no undo anyway (yet).
Undo should be pretty easy as long as you can represent easily represent user actions in a transactions stack. A user doing something will then cause an action to be pushed onto the stack (although you would probably want some coalescing so that you don't have to undo each character you typed, you can just undo a sentence). The undo command will pop a transaction of the stack and apply the inverse. I've written a fairly nice implementation of this before in Java and it didn't take me very long.
The code looks good. It looks like you have the basic actions nailed. However, there are a few superfluous typecasts. In particular, please remove the ALLOC_OBJ macro because the name seems to suggest it does more than it actually does. You may want to use the existing doubly-linked list implementation at include/wine/list.h as it has been well-tested and people are familiar with the API. This is just a suggestion and you may feel that it is easier to use your own. A few more comments wouldn't go amiss either.
What is the ParaStyle structure used for? How does this relate to the Style structure? Each character can have a different style, so why is there a paragraph style?
Rob
Rob Shearman wrote:
No, the LocalAlloc/LocalFree/GlobalAlloc/GlobalFree functions are relics from the old days of Win16. You should use HeapAlloc/HeapFree.
True.
Undo should be pretty easy as long as you can represent easily represent user actions in a transactions stack.
Yes, I know the undo basics. The most recent source has the basic stack structure. :) It's still a bit complex, as, first, we have formatting to store on the stack (linked actions are the way to go), and it's easy to miss something, making undo fail in unusual circumstances.
stack (although you would probably want some coalescing so that you don't have to undo each character you typed,
There's more: it's required by RICHEDIT API.
sentence). The undo command will pop a transaction of the stack and apply the inverse.
Well, it can't apply the inverse ;) You need to push the inverse.
However, there are a few superfluous typecasts.
Any examples ?
In particular, please remove the ALLOC_OBJ macro because the name seems to suggest it does more than it actually does.
I'd prefer to rename the macro, and not remove it completely, it could be used for tracking memory allocations one day. And I don't really like the HeapAlloc API.
This is just a suggestion and you may feel that it is easier to use your own.
The big advantage is that it's independent from Wine-specific API. Which makes it more useful in non-Wine applications (the code is LGPLed, so why not use it somewhere else ?).
A few more comments wouldn't go amiss either.
Definitely. And to make things worse, not all FIXME's and XXXKF's are still relevant. It asks for a cleanup. But, undo first (because it involves even more major changes).
What is the ParaStyle structure used for?
Currently, it's not used. It's a placeholder for keeping real paragraph attributes, RICHEDIT's PARAFORMAT/PARAFORMAT2, part of which is currently stored directly in ME_Paragraph (nAlign, nLeftMargin, nRightMargin, nFirstMargin). The whole paragraph handling (para.c) needs lots of work.
Each character can have a different style, so why is there a paragraph style?
See above. The current content of the structure is irrelevant, and is a kind of legacy from my misunderstanding of the relationship between CHARFORMAT and PARAFORMAT.
Krzysztof
Krzysztof Foltman wrote:
Rob Shearman wrote:
However, there are a few superfluous typecasts.
Any examples ?
There are a few HGDIOBJ casts as well as the already mentioned ALLOC_OBJ macro. For example, in paint.c: DeleteObject((HGDIOBJ)hbr);
In particular, please remove the ALLOC_OBJ macro because the name seems to suggest it does more than it actually does.
I'd prefer to rename the macro, and not remove it completely, it could be used for tracking memory allocations one day. And I don't really like the HeapAlloc API.
What is the ParaStyle structure used for?
Currently, it's not used. It's a placeholder for keeping real paragraph attributes, RICHEDIT's PARAFORMAT/PARAFORMAT2, part of which is currently stored directly in ME_Paragraph (nAlign, nLeftMargin, nRightMargin, nFirstMargin). The whole paragraph handling (para.c) needs lots of work.
Ok.
Each character can have a different style, so why is there a paragraph style?
See above. The current content of the structure is irrelevant, and is a kind of legacy from my misunderstanding of the relationship between CHARFORMAT and PARAFORMAT.
Great. The code looks good so far. I look forward to being able to review a patch against Wine.
Rob
Rob Shearman wrote:
There are a few HGDIOBJ casts as well as the already mentioned ALLOC_OBJ macro. For example, in paint.c: DeleteObject((HGDIOBJ)hbr);
Yes, both are HANDLEs, so they don't really need a cast. I've probably written too much MFC code in my life :)
I've deleted all HGDIOBJ casts, but I'm not sure if it's the only case of redundant casts.
Krzysztof
Rob Shearman wrote:
What is the ParaStyle structure used for? How does this relate to the Style structure? Each character can have a different style, so why is there a paragraph style?
Ahh, good news: the paragraph format structure (PARAFORMAT2) doesn't need any precalculation (contrary to the character style, which caches a HFONT and TEXTMETRIC). That means I can keep the paragraph format directly in the ME_Paragraph, and remove the ME_ParaStyle completely.
In fact, I've just removed all the ParaStyle-related code without any negative consequences.
Krzysztof
Krzysztof Foltman wrote:
My rich text control is still far from even semi-complete, but I think too much is done to start over, so I'm releasing it today. The number one reason of its incompleteness is that I put more emphasis on making things work exactly like in the original than on implementing more functions.
Sorry to be barging in like this and maybe it is all Old news. But ... I have been using in a few projects, Windows side and Linux, a grate rich edit control, I'm sure every body knows, and used even if he doesn't know. "Scintilla"! It has anything a RichEdit control needs (and much more but that does not have to be used). It even has the export to RTF function. (And to HTML for clipboard operations) It has 95% EditBox emulation. It is very stable, heavily used and debugged, and highly portable. The License is a BSD type license so I don't think it will be any problem. The only things missing are the RTF parser and the MS-RichEdit emulation (Data types and message translations). But it looks like these two are what above patch and existing code has. I am almost positive that if such a parser (And emulation layer) is submitted to the Scintilla project it will be farther maintained by the project. One more thing good about it, that makes a lot of sense, is its popularity, which means there are a lot of people that know this code-base and can help fix problems. (And it does compile under wine out of the box.)
Please forgive me if all this is "Old News". I didn't check the archives. Any way just my $0.02.
Free Life Boaz
-
Boaz Harrosh -
Krzysztof Foltman -
Mike McCormack -
Rob Shearman