Looks good, thanks. There is a problem with inline components: http://bugs.freepascal.org/view.php?id=11069
Please delete 11068, it was posted private by accident.

Martin

Can you also commit tests for these fixed bugs (11059, 11060, 11067) to svn?

Jonas

Things now seem to work for me after a little adjustment. I don't know if it is intentional, but the order in which references are satisfied seems to have been reversed in the current version, which revealed a minor error in my code. I suspect I won't be the only one, so it might be kind to reverse the list to avoid similar problems.

The other thing that struck me is that unsatisfied references do not throw an error - shouldn't they? (It would have made fixing the error a little easier) How about the following patch:

Colin

I converted them to fpcunit tests while implementing, but since I don't know yet how to handle those in the testsuite, I didn't commit them yet.

Michael.

OK, I knew how to handle visual form inheritance so I could fix 11067, but inline I have no idea what it is. Can you explain me what needs to be done, and in what situations it is used ?

Alternatively you can supply a patch ;-)

Michael.

Nono, it's good that you fixed your bug.

You should never rely on the loading order. For forms, for instance, it is dependent on the creation in the IDE. A simple cut and paste could change the order. Relying on this would be bad practice.

The other thing that struck me is that unsatisfied references do not throw an error - shouldn't they? (It would have made fixing the error a little easier) How about the following patch:

Delphi doesn't throw an error either, and it should definitely not do so at the end of local reference resolving; At most this could be done after all loading is done, and you don't know when that is: forms can be created at any time...

Michael.

Absolutely

It still seems strange not to throw an error. The point I suggested is the first time that it is definitely known that the local reference will never be resolved, as at that point the reference is simply thrown away if not resolved. (Or am I missing something?)

Colin

Inline is used for subforms, components in a form which inherit from another form (TFrame in Delphi), in MSEgui every form can be used as inline component. The ffInline filer flag must be written and the ancestor must be looked up on writing and reading like inherited components.

Alternatively you can supply a patch ;-)

I don't think I am "clean" enough...

Martin

Alternatively, you can write the test case ;-)

Vincent

I'd like to second Colins wish. Please use the same resolving order as Delphi. MSEide+MSEgui compiles with Delphi7 so the reversed order is an unnecessary complication and error source.

Agreed.

Thanks, Martin

I'll do. The first is
http://bugs.freepascal.org/view.php?id=11069

Martin

It's not a complication. Resolving order is and must be irrelevant. Until 'Loaded' is called, you're not supposed to make any assumptions about what property is and what property is not set, and hence also not about the order.

Michael.

OK, So:

 Form1 = TForm1
  Frame1 = TMyFrame [inline comes here somewhere]
  end
 end

When Frame1 is streamed, where do I find TMyFrame ? It's Ancestor is not being streamed ?

Michael.

I have noticed another incompatibility with the previous version - the TParser object in parser.inc can now return toWString as one of the possibilities; the previous version returned toString regardless, but offered a wide and normal version of the string. This change breaks lazarus on wide strings. Which should be changed?

Colin

toWString was missing, now it's more delphi compatible and you don't have to check every character of the widestring (returned with TParser.TokenWideString) to determine if the string is really a wide or an ansi string.
Note that now you still have a normal and wide version of the string (TokenString / TokenWideString) but Token property can be toWString too. So the only needed change should be in checking for toWString too when checking for toString. Ok, toWString is declared in trunk only, so you should use an ifdef for now. However it will be merged to fixes_2_2 too, so the ifdef will be only needed till 2.2.2 is released.

Giulio
_________________________________________________________________ News, entertainment and everything you care it now! http://www.live.com/getstarted.aspx

Hello,

at the end of last year, the FPC team was notified that certain portions of the FPC codebase contained possible copyright infringements on copyrighted code from Codegear (Borland).

As a precaution, an internal audit was done by the FPC team using an automated tool, and a list of routines was compiled that could possibly be considered a copyright infringement. The complete list is at the end of this mail.

It was decided that these routines should be re-implemented with cleanroom implementations, to remove all suspicion from the FPC codebase.

The FPC team has now finally completed creating cleanroom implementations, and the implementations have been committed to the main branch (trunk) of the FPC codebase. Later, these will be merged to the fixes branch, and release 2.2.2 will be created with this implementation.

While extensive testing has been done (using the FPCunit testing framework), and the working of Lazarus with this new code was verified, it is possible that bugs remain. We would therefor very much appreciate that if you have code that uses some of the routines below, that you test your code with the latest FPC from subversion (revision 10572 or above) and report any bugs that you may encounter. If you do, please submit a bugreport in mantis:

http://bugs.freepascal.org/

We'll give the code 2 weeks to be tested, and unless major problems pop up, the code will be merged to the fixes branch, and then release 2.2.2 will be started with this code. It is therefor vital that the code be tested very well, as it will definitely end up in the next release.

For the FPC team,

Michael.

-----------------------------------------------------------------------

Complete list of replaced routines:

Classes unit:
-------------

ExtractStrings
TParser.TokenComponentIdent
TParser.NextToken
TWriter.WriteComponent
TWriter.WriteComponentData
TWriter.WriteProperties
TStringList.Grow
TStringList.QuickSort
TStringList.Find
FindNestedComponent
GlobalFixupReferences
GetFixupInstanceNames
RemoveFixupReferences
RedirectFixupReferences
GetFixupInstanceNames
RemoveGlobalFixup
RemoveFixups
TReader.DoFixupReferences
TReader.FindComponentClass
TReader.ReadCollection
TList.Extract
TFPList.Assign
TCollection.GetNamePath

Strutils unit:
--------------
SearchBuf
DecodeSoundexInt

Varutils unit:
--------------
DoVarClearArray

Math unit:
----------
MomentSkewKurtosis

ComObj unit:
------------
HandleSafeCallException

DB unit:
--------
TDataset.CalculateFields
TDataset.DataEvent
TDataset.EnableControls
TDataLink.CalcFirstRecord
TField.CalcLookupValue
TField.RefreshLookupList

http://bugs.freepascal.org/view.php?id=11059
Does this cause no problems for Lazarus?

Martin

Yes, it does. I didn't notice before though.

It is a problem for non-control TComponent you drop on a form, like a TOpenDialog.

Vincent

I added this and 11060 to the cleanroom page.
http://wiki.lazarus.freepascal.org/FPC_Cleanroom#Bug_reports

There still is a memleak in the reader, when starting and stopping Lazarus: 333861 memory blocks allocated : 48899445/49814032 333803 memory blocks freed : 48898000/49812384 58 unfreed memory blocks : 1445

Vincent

11059 should be fixed.

I think I plugged that one (Vincent: we had it in the tests too, if you recall). Can you test please ? If it persists, please send me the heaptrc output, that will make searching easier.

11060 is a harder nut to crack, though.

Michael.

It is not yet fixed. I sent heaptrc to you privately.

Vincent

Fixed in 10583:
Heap dump by heaptrc unit
338700 memory blocks allocated : 45274460/46194864 338700 memory blocks freed : 45274460/46194864 0 unfreed memory blocks : 0

Vincent

There seems to be a problem in TReader.FindComponentClass - the search in FindInFieldTable does not properly include ancestors.

See attached patch

Colin

C Western wrote

There seems to be a problem in TReader.FindComponentClass - the search in FindInFieldTable does not properly include ancestors.

Great, I applied the patch. This should probably fix issue 11060 !

Michael.

There also seems to be a problem with fixing up references to other components while reading - TLabel.FocusControl seems to be one of the things that is tripping this up. Local unresolved references are being silently discarded, but I think should be added to the global list. The attached patch fixes it for my application, but might not be the best general fix.

Colin

----- Original Message -----

Indeed. It completely short-circuits the idea of trying to locally resolve references before trying globally :)

Anyway, your hint gave me the idea where to look.
I fixed the issue properly. The error was in another routine altogether.

Hopefully now 11060 is really fixed.

Michael.

<snip>

Other dialogs seems to be fine though.

Jesus just told me that it is fixed in r10587.

Vincent

Great, I set the bug to resolved.

Seems like the most obvious bugs are fixed.
Now I wait for the subtle ones to appear ;-)

Michael.

Here it is the first one:
http://bugs.freepascal.org/view.php?id=11067
(Wrong ancestor value in defineproperties).

11060 seems to be fixed for MSEide too, thanks a lot.

Martin

Seems fixed now.
I hope I have done it correct (your test program runs fine) and didn't break anything else... :(

Michael.

It is like streaming of an inherited form. Lookup the ancestor and write the property differences.
On reading TMyFrame is found by TReader.FindComponentClass (normally by GetFieldClass) as with non inlined child components. TMyFrame.Create loads the TMyFrame instance with the initial form resource data.

Martin

Nono, when streaming an inherited form, the IDE provides the ancestor instance when the streaming starts, because it calls writedescendent.

When you have a frame inside a form IDE, the IDE cannot provide the ancestor of the frame: the streaming code must look for the original instance of the frame by itself. My guess is that this is done by setting the OnFindAncestor callback ?

Michael.

I am not convinced. By controlling the order of the properties and the load order of the forms it is possible to control the resolving order. Does FPC loose on quality if you use the same order as Delphi? It is possible that we don't know all future applications of resolving order at the moment, why should we give away compatibility if not necessary? In my developments I try to constrict the possibilities as little as possible. Often the users of our code do things we never thought. :-)

Martin

Maybe this patch (for lazarus) solves the issue? (warning: lazarus compiles and runs but I didn't test it).

Giulio

_________________________________________________________________ Discover the new Windows Vista
http://search.msn.com/results.aspx?q=windows+vista&mkt=en-US&form=QBRE

Correct. Sorry that I didn't get your question right.

Simply said: if I must reverse order, then I have to redo the whole cleanroom stuff and it will be exactly as in Delphi. This is not an option.

And since resolving order is not something you are supposed to rely on, you're on dangerous ground. As soon as Delphi changes it, your code will break anyway.

As I said: the only moment that is guaranteed is the 'loaded' call, stick to that.

Michael.

Op maandag 31-03-2008 om 16:13 uur [tijdzone +0200], schreef Michael Van Canneyt:

Simply said: if I must reverse order, then I have to redo the whole cleanroom stuff and it will be exactly as in Delphi. This is not an option.

Is this about mine (cleanroom) code? Maybe I can change it? If someone can explain to me what chould be changed and supply some tests... I haven't looked into the differences.

Joost.

I probably don't understand. What is the problem to reverse the iterating order over a list?

Martin

It's a single linked list. Not a TList. Reversing it would be a waste of time.

Michael.

It seems to work reasonably OK for me. It doesn't handle the copyright symbol quite right that was causing me problems, but I don't think the previous version did either. It does stop the error message though.

Colin

Giulio, thanks for the patch.

Colin, thanks for testing.

Applied in r14698.

Vincent

Fixed;
There is no test to see what happens when you stream a descendent of a form with an inline form;
if you have such a case, please test it.

Michael.

Argh, I just made it too!
OK, I'll update from trunk and test it. There are interesting cases with inlined inherited components and the like.

Thanks, Martin

You used my copyrighted code, the sources look exactly the same. ;-)

Martin

A strong case against all these copyright suits... :-)

Michael.

And we made the same error, see attachment. It is a little bit more complicated.
I hope I got the coding style right. ;-)

Martin

Not quite, I adapted it, no big deal.

It's like I said when I fixed the original issue:
for inherited forms with inline forms, the original patch was not tested.

Anyway, I applied your patch with a slight modification, thank you very much ! Revision 10601.

Michael.

The next problem is childpos handling:
http://bugs.freepascal.org/view.php?id=11102

Martin

I've never understood what this is about, so some additional information would be welcome...

Michael.

It is used in order change the widget stacking order in an inherited form for example:
In ancestor form streaming- child- and creation-order is twidget1, twidget2, twidget3 what means (in MSEgui, I don't know for Lazarus) twidget2 is over twidget1, twidget3 is over twidget2.
Now you do 'Bring to Front' for twidget1 in a inherited form which changes the child order to twidget2, twidget3, twidget1.
The new child order must be written and read back while streaming the descendent. It is done with ffChildPos filerflag and a following integer. Repositioning of the inherited children is done by TComponent.SetChildOrder IIRC.

Martin

Bweeh :(

That means I must compare original and new child order.

OK, thanks. I'll try to fix this ASAP.

Michael.

Zitat von Martin Schreiber :

Same for lazarus.

Mattias