issue-#63 fixes in StdStamps

[Ivan Denisov]

Last version with list is more clear to use, I like it.

Josef, I fix the problem with diffs.
http://redmine.blackboxframework.org/pr ... 2beb94c5fd

It was my fault. I have deleted the odcread application accidentally.

[Robert]

I have used version 269 on a Test version of BlackBox for a few days with no problems.

I have just installed it onto my main version to test it in the wild. BlackBox now crashes on initial loading, before the proper Trap handler is installed, which makes it hard to investigate the problem. I am currently working on something else against a deadline - Scream!!!!!

Will report more later.

[Zinn]

Robert, you increased the number of entries in your versions of StdStamps.
Documents with more than 25 entries in the stamp will crash with version used here.
- Helmut

[Robert]

Robert, you increased the number of entries in your versions of StdStamps...

I want to discuss this point later, but I don't think it is todays problem (I have taken active steps to avoid this as a problem).

I think the problem is in the procedure GetData. Josef has removed the test from my suggestion for testing if history[.].comment = NIL. This seems like an error.

He has also removed Oms's test "If entryno <= v.entries". Why?. Is this a good idea?


I also note that since the signature of GetData has changed other modules need to be altered to stay compatible. If we had added a new procedure (eg "GetAdditionalData") other modules that don't use it would remain compatible.
This is not a real problem for me, just something to think about.

Regards

Robert

[Josef Templ]

I have fixed the bug in GetData with the missing NIL test.
This check was unintentionally removed, i.e it was forgotten while
merging GetData and GetMoreData.

The test for entryno overflow was removed intentionally because in
Robert's proposal as well as in BB1.6 the overflow was ignored which it should not be.
However, my solution was oversimplified because the history array is of a fixed size and may only be filled partially.
Therefore, the correct way to handle it is an ASSERT, which I have also added now.

Please see the diffs at http://redmine.blackboxframework.org/pr ... ee44967aa0.

- Josef

[Robert]

Ok, I think we now have a good solution; compared to the February proposal the main user interface is definately better.

An interesting difference between Josef's approach and my proposal is the emphasis on "Forward Compatibility". (All the proposals had full back compatibility, none have full forward compatibility as wide comments cannot be read by old software.)

My approach was to compromise on forward compatibility in the interests of simplicity; Josef's approach is to maximise forwards compatibility at the expense of some additional code. Helmut shared Josef's approach. I don't have a real problem with it.

I want to suggest we put in a hook to support forward compatibility for some future potential expansion. I am thinking that for a later release we might want to provide additional entries, or the option for a variable number of entries. To support this with Version three code (thereby maximising forward compatibility for such an enhancement) we should add the lines now:

Code: Select all

         VAR z: INTEGER; comment: Comment;

					IF i < maxHistoryEntries THEN
						... current code ...
						ELSE
							rd.ReadInt(z);	(*  fprint  *)
							rd.ReadInt(z);	(*  snr  *)
							rd.ReadInt(z);	(*  date  *)
							rd.ReadXInt(z);	(*  time  *)
							rd.ReadXInt(len);
							IF len > 0 THEN
								IF version >= wideCommentVersion
									THEN rd.ReadString(comment)
									ELSE rd.ReadXString(comment) END
							END
						END
                 end                                        ------    already exists
                 v.entries := MIN(v.entries, maxHistoryEntries);

inside the FOR i loop in Internalize.


A different comment I have regards the Documentation. To use the exported procedures GetData & Stamp the programmer really needs to know that the maximum size of comments is 63 characters.

Rather than writing a full document I suggest that simply exporting the type Comment (ie add 1 asterix) gives the programmer adequate clues without having to read the source code.


Note added later:

Actually Comment should be exported anyway. It should be the TYPE used by any application code that calls GetData.

[Josef Templ]

Comment is now exported.
The size of the history array is now dynamic, so it is prepared for future expansions
and it will also read in any of Robert's internally used stamps with a larger history.

For the changes see http://redmine.blackboxframework.org/pr ... e31c799e05.

- Josef

[Robert]

There seems to be a problem with copying newly created stamps.

I can't quite figure it out, but maybe before a Stamp is ever saved v.nentries = 0, and so the date is not copied in CopyFromSimpleView.

Should the loop be

Code: Select all

FOR i := 0 TO MAX (v.nentries - 1, 1) DO

?

Do we need to copy v.comment0?


Sorry!

Robert

[Josef Templ]

Robert, thanks for pointing out this anomaly.
Your proposal for the fix is just perfect.
Also, for the sake of completeness, the comment0 field should be copied
in CopyFromSimpleView. In practice, it will not make a big difference, though.

Unless there are any objections I will make the changes and push them to master.

- Josef

[Robert]

Josef

If you are making these minor changes you might also consider deleting the unnecessary forward declaration of Update as suggested by Helmut?

Robert