[patch]: replace form::Form and remove form.{cc,hh}
The source files dillo2/src/form.{hh,cc} (which define the form::Form class) are modified copies of dw2/test/form.{hh,cc} . Dillo uses almost none of the code in these files. All dillo needs is a Receiver class that wraps a DilloHtmlForm and delegates calls to the event handlers to DilloHtmlForm methods. This patch removes all references to form::Form and form.{hh,cc} . It replaces form::Form with DilloHtmlReceiver. It also moves the event handler code into a DilloHtmlForm method and removes some unnecessary (void *) casting. This simplifies the code that bridges form events to DilloHtmlForm methods. WARNING: developers must run " make maintainer-clean ; ./autogen.sh " after this patch appears in their CVS working directory. This is because automake has to recreate Makefile.in , otherwise the build will try to compile form.cc which no longer works. After this patch is applied you can delete dillo2/src/form.{hh,cc} . Don't delete dw2/test/form.{hh,cc} though! Please comment! Regards, Jeremy Henty
On Sat, May 31, 2008 at 05:33:50PM +0100, Jeremy Henty wrote:
The source files dillo2/src/form.{hh,cc} (which define the form::Form class) are modified copies of dw2/test/form.{hh,cc} . Dillo uses almost none of the code in these files. All dillo needs is a Receiver class that wraps a DilloHtmlForm and delegates calls to the event handlers to DilloHtmlForm methods.
This patch removes all references to form::Form and form.{hh,cc} . It replaces form::Form with DilloHtmlReceiver. It also moves the event handler code into a DilloHtmlForm method and removes some unnecessary (void *) casting. This simplifies the code that bridges form events to DilloHtmlForm methods.
WARNING: developers must run " make maintainer-clean ; ./autogen.sh " after this patch appears in their CVS working directory. This is because automake has to recreate Makefile.in , otherwise the build will try to compile form.cc which no longer works.
After this patch is applied you can delete dillo2/src/form.{hh,cc} . Don't delete dw2/test/form.{hh,cc} though!
Committed. I removed form.{hh,cc} from cvs. Although the may reappear once html.cc is split. -- Cheers Jorge.-
On Sun, Jun 01, 2008 at 01:42:11PM -0400, Jorge Arellano Cid wrote:
I removed form.{hh,cc} from cvs. Although they may reappear once html.cc is split.
Well, since you mention it...! :-) I had another go at splitting html.cc and this time I got it to work. No code was modified except that DilloHtml::getCurrentForm is no longer inline and the DilloHtmlForm constructor and destructor are wrapped in Html_form_{create,delete}, which can be called without access to the DilloHtmlForm class definition. So let's talk filenames! I moved the form code into new files called html_form.{hh,cc} . I really like hierarchical file names like that (they make related things sort next to each other), but maybe you would prefer the old form.{hh,cc} ? Also, I had to move a lot of declarations that both html.cc and html_form.cc use into a new header called html_internal.hh . Again, please specify a different file name if you want. I hadn't planned on creating a second new header but there are several definitions that are used by most of the html.cc internals, eg. almost everything needs the DilloHtml class definition and various HT2* macros etc. The "internal header" factors out that common API. I'd like to avoid it but it seems inevitable to me. Note that adding html_internal.hh is a one-off change. Further splitting should not require extra internal headers, just one new header per new source file as is standard. Also note that splitting off the form code is particularly messy since it includes new objects that appear in the DilloHtml definition. I would expect further splitting (eg, the table code) to be much simpler. The final decision is what to do with function names that move from *.cc to *.hh files. Leave them the same or rename "Html_*" to "a_Html_*"? I would suggest leaving them the same because they are still Html internals. They only appear in *.hh files because the internal implementation is spread among several files. If people think this sounds good and confirm the right names for files and functions then I will start proposing patches. I will move the code around in stages without breaking the build, so you'll get several small patches instead of one big one. I think that will be easier to review (but you can have a mega-patch if you really want one). Here are the stats: before: $ wc html*.{hh,cc} 21 42 338 html.hh 0 0 0 html_form.hh 0 0 0 html_internal.hh 6016 20229 191216 html.cc 0 0 0 html_form.cc 6037 20271 191554 total after: $ wc html*.{hh,cc} 21 42 338 html.hh 40 121 1404 html_form.hh 240 841 7832 html_internal.hh 4114 14090 129091 html.cc 1764 5563 57036 html_form.cc 6179 20657 195701 total (The final version may differ as I clean things up, but it won't be much different.) Regards, Jeremy Henty
OK, I'll kick this off with a small patch that wraps the DilloHtmlForm constructor and destructor in an API that doesn't need the class declaration. The next stage will be to create stubs for the new files and hook them into the existing source and build system. That depends on deciding what the new file names should be. My thoughts: The form implementation: form.cc or html_form.cc . The form header: form.hh or html_form.hh . The shared API for Html module internals: html_common.hh , html_shared.hh , html_internal.hh or html_internals.hh . Please comment! Regards, Jeremy Henty
On Mon, Jun 02, 2008 at 09:23:44PM +0100, Jeremy Henty wrote:
OK, I'll kick this off with a small patch that wraps the DilloHtmlForm constructor and destructor in an API that doesn't need the class declaration.
Committed.
The next stage will be to create stubs for the new files and hook them into the existing source and build system. That depends on deciding what the new file names should be. My thoughts:
The form implementation: form.cc or html_form.cc .
form.cc
The form header: form.hh or html_form.hh .
form.hh
The shared API for Html module internals: html_common.hh , html_shared.hh , html_internal.hh or html_internals.hh .
html_common.hh -- Cheers Jorge.-
The first patch in this set creates stubs for the new files and adds them to the build. Developers should run " make maintainer-clean ; ./autogen.sh " after checking this out so that the new source file is compiled. The other patches move some macros, typedefs, structs and class definitions from html.cc into the new headers. The next step is to move some of the function declarations across, but that depends on deciding what to do with the function names. Should I rename "Html_*" to "a_Html_*"? In other words, do functions in form.hh count as part of the Html module API or part of its internals. I am inclined to say they are internal and so not rename them. Comments? Regards, Jeremy Henty
On Wed, Jun 04, 2008 at 03:38:38PM +0100, Jeremy Henty wrote:
The first patch in this set creates stubs for the new files and adds them to the build. Developers should run " make maintainer-clean ; ./autogen.sh " after checking this out so that the new source file is compiled.
The other patches move some macros, typedefs, structs and class definitions from html.cc into the new headers.
Committed.
The next step is to move some of the function declarations across, but that depends on deciding what to do with the function names. Should I rename "Html_*" to "a_Html_*"? In other words, do functions in form.hh count as part of the Html module API or part of its internals. I am inclined to say they are internal and so not rename them. Comments?
I'd prefer to have C functions called from other sources as a_Html_*. -- Cheers Jorge.-
On Thu, Jun 05, 2008 at 09:02:28AM -0400, Jorge Arellano Cid wrote:
On Wed, Jun 04, 2008 at 03:38:38PM +0100, Jeremy Henty wrote:
The next step is to move some of the function declarations across, but that depends on deciding what to do with the function names. Should I rename "Html_*" to "a_Html_*"?
I'd prefer to have C functions called from other sources as a_Html_*.
OK, but how does that rule apply to Html_tag_{open,close}_*() ? They aren't explicitly called anywhere, just referenced from the Tags array in html.cc . The split will move some of them into form.cc . It would be ugly to rename some of them but not others. If the plan is to move most or all of them to separate files like form.cc (which I think would be a good way to split up html.cc) then perhaps we should rename them all to a_Html_tag_{open,close}_*() in advance? I'll submit more patches as soon as this is decided. Regards, Jeremy Henty
On Thu, Jun 05, 2008 at 03:48:53PM +0100, Jeremy Henty wrote:
On Thu, Jun 05, 2008 at 09:02:28AM -0400, Jorge Arellano Cid wrote:
On Wed, Jun 04, 2008 at 03:38:38PM +0100, Jeremy Henty wrote:
The next step is to move some of the function declarations across, but that depends on deciding what to do with the function names. Should I rename "Html_*" to "a_Html_*"?
I'd prefer to have C functions called from other sources as a_Html_*.
OK, but how does that rule apply to Html_tag_{open,close}_*() ? They aren't explicitly called anywhere,
They are called by the parser from inside html.cc.
just referenced from the Tags array in html.cc . The split will move some of them into form.cc . It would be ugly to rename some of them but not others. If the plan is to move most or all of them to separate files like form.cc (which I think would be a good way to split up html.cc) then perhaps we should rename them all to a_Html_tag_{open,close}_*() in advance?
As the Html_tag_{open,close}_*() functions are closely related to the parser, I'd prefer to have them not renamed. If there's a need, these functions could call a_Form_*() functions. Something like: Html_open_form () { [...] a_Form_do_something() [...] } -- Cheers Jorge.-
participants (2)
-
jcid＠dillo.org -
onepoint＠starurchin.org