Page 4 of 5
Re: issue-#63 fixes in StdStamps
Posted: Fri Oct 23, 2015 4:30 pm
by 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.
Re: issue-#63 fixes in StdStamps
Posted: Tue Oct 27, 2015 10:00 am
by 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.
Re: issue-#63 fixes in StdStamps
Posted: Tue Oct 27, 2015 10:19 am
by 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
Re: issue-#63 fixes in StdStamps
Posted: Tue Oct 27, 2015 10:56 am
by Robert
Zinn wrote: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
Re: issue-#63 fixes in StdStamps
Posted: Tue Oct 27, 2015 7:08 pm
by 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
Re: issue-#63 fixes in StdStamps
Posted: Tue Oct 27, 2015 8:18 pm
by 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.
Re: issue-#63 fixes in StdStamps
Posted: Wed Oct 28, 2015 8:58 am
by 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
Re: issue-#63 fixes in StdStamps
Posted: Wed Nov 04, 2015 10:10 pm
by 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
Re: issue-#63 fixes in StdStamps
Posted: Thu Nov 05, 2015 10:35 am
by 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
Re: issue-#63 fixes in StdStamps
Posted: Thu Nov 05, 2015 11:50 am
by 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