issue-#63 fixes in StdStamps

[Robert]

There are surprises wherever one looks, in the old version as well as in the new version.
Surprisingly tricky and buggy stuff in this module.

Yes. My first post on the subject described it as

This year I decided to investigate closer, and came to the conclusion that it made no sense because it was riddled with bugs.

The old version does not stamp when an empty file is saved for the first time.
Is this a feature or is it a bug?

As I said in my second posting above, definitely a bug.

[Josef Templ]

This issue is not yet resolved.

- Josef

[Ivan Denisov]

This issue is not yet resolved.

Sorry, I returned it back.

[Robert]

In topic http://forum.blackboxframework.org/view ... =265#p2361 Berhard wrote

Bobs last post (3. Feb. 2015) contained:
"I have also now discovered two new bugs, which have always been there."
I guess/hope that Bob's Version above includes these "unpublished" changes, but I don't know.

In fact that version was published here on 13-Feb-2015 in topic http://forum.blackboxframework.org/view ... =186#p1886. This is exactly the same version as I recently published in topic http://forum.blackboxframework.org/view ... =265#p2365.

These versions differ from the Zinn / CPC version being discussed here with regard to what Chris calls "Forward compatibility", the topic of the second link above. This topic never reached an agreed decision.

I would like to reopen this topic here as I think that the "benefits" of partial forward compatibility do not justify the additional complexity, and in fact have some disadvantages. (Both versions do provide full "Back" compatibility; that is not being questioned.)


I was intending to open this compatibility discussion after any modifications and improvements to the current proposal had been agreed. But as I will be out of contact for the next three weeks I decided to open it before this thread is closed.

The argument is: If we write a Stamp in BlackBox 1.7, must it be fully functional in BlackBox 1.6? If so, then we cannot provide for 16-bit characters in Stamps. But we have already decided to do that, so full forward compatibility is a lost cause.

All versions provide a high level of forward compatibility. The Document is readable / editable /saveable in BlackBox 1.6, except that the Stamp itself is unavailable (it is an Alien). When the Document is reopened in BlackBox 1.7 the Stamp comes back to life. This represents a very small level of inconvenience, as people who write / develop on 1.7 will only occasionally and briefly have reason to refer to version 1.6. Stamps are mainly used by their authors, I can't imagine someone publishing Documents from 1.7 where a wide audience of 1.6 users need to open the Stamps.

The CPC version includes the additional logic to save a Stamp that only contains 8-bit characters in BlackBox 1.6 format. This provides some small additional level of forward compatibility. It comes at the cost of extra code complexity.

However I see two problems. We now have two classes of users; People who's native alphabet is 8-bit who have an "advantage" over people who's alphabet is 16-bit. I think this division is undesirable.

Even people (myself!) who appear to live in the 8-bit world do frequently use other unicode characters. I might write "Changed this → that", or I might use a Greek symbol to describe an equation. Even if most of my comments are only 8-bit, it only takes 1 non 8-bit character to loose my forward compatibility "advantage". I might be deterred to write the comment I want in order to keep my "advantage".

Better, and simpler, to make the change to 16-bit completely.


I do have another point, which I shall not discuss in detail here. If we accept that 1.7 Stamps are not fully forward compatible to BlackBox 1.6 we have the option to change the number of comments from 25 (at present) to 35, or another number, or a variable number. If this is considered to be too much of a feature to be considered just now, I will still argue to make the code capable of supporting a variable number (it involves writing 1 extra number to disc) so that if we subsequently (BlackBox 1.8 ?) do agree to change the number of comments we can without breaking back or forward compatibility from 1.7.

[Josef Templ]

I have added the changes proposed by Robert in a form that preserves the previous behavior as much as possible.

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

@Robert: could you please try it out and let me know if there is something missing.
The download is under http://blackboxframework.org/unstable/i ... a1.261.zip.

- Josef

[Robert]

@Robert: could you please try it out and let me know if there is something missing.

I will look at this carefully; it may take a few days!
Regards

[Robert]

An initial, off topic (?) observation.

I have started using F9 to compare this centre version to the experimental one I am using daily, and it picks up lots of irrelevant differences. These are caused by the center version having more ";"s.

The BlackBox programming conventions state

Semicolons are used to separate statements, not to terminate statements. This means that there should be no superfluous semicolons.

Are we ignoring this by accident, or deliberately?

[cfbsoftware]

If it is by accident or the developer does not properly understand the use of semicolons then they should use the semicolons option of the DevAnalyzer tool to check their work.

[Josef Templ]

In StdStamps there are no changes in the usage of semicolons in any line that is not affected by the issue itself.
So, Robert, if there are many such changes in your version, they must have been introduced by yourself.

This clearly shows that it is not a good idea to mix separate issues together
because then you cannot easily see the differences that are related to one particular issue.
The usage convention of semicolons is such a completely different issue and it is one that affects
many many lines in many files. Don't touch it. In addition, it is highly questionable if the recommended
programming convention is still valid in the context of line-based diff tools. If you add a line you have to add
a semicolon in the line before, which changes two lines where essentially only one line is changed.
This makes it harder to see the real difference between two versions. We should keep the focus on
bug fixes and features, i.e. on real problems instead of spending our time on questionable source code
cosmetics.

- Josef

[Robert]

A few minor comments for now.

I take the view that since we are contemplating a fairly major modification, we might as well take the opportunity to do a general cleanup. While I think StdStamps is a good and useful idea, this implementation was not Oms's finest hour. The first two comments below come into this category.

The third comment is more serious.

1 - The outer WITH below is unnecessary, and rather confusing - just delete it.

Code: Select all

	PROCEDURE (v: StdView) HandlePropMsg (VAR msg: Properties.Message);
		VAR font: Fonts.Font; asc, w: INTEGER;
	BEGIN
		WITH msg: Properties.Preference DO
			WITH msg: Properties.SizePref DO
				SizePref(v, msg)
			| msg: Properties.ResizePref DO
				msg.fixed := TRUE
			| msg: Properties.FocusPref DO
				msg.hotFocus := TRUE
			| msg: TextSetters.Pref DO
				font := FontContext(v);
				font.GetBounds(asc, msg.dsc, w)
			ELSE
			END
		ELSE
		END
	END HandlePropMsg;

2 - It is simpler, and more efficient (!), to declare v to be of type StdView and delete the type guard WITH.

Code: Select all

	PROCEDURE SetComment*;
		VAR v: Views.View; op: SetCmtOp;
	BEGIN
		v := GetActiveStamp();
		IF v # NIL THEN
			WITH v: StdView DO
				NEW(op); op.stamp := v; op.age := comment.age;
				NEW(op.oldcomment, LEN(comment.s$) + 1);
				op.oldcomment^ := comment.s$;
				Views.Do(v, setCommentKey, op)
			END
		END
	END SetComment;

The procedure GetData is useful (I use it!), but incomplete. We also need (I am NOT suggesting we keep the name below!):

Code: Select all

	PROCEDURE GetMoreData* (v: Views.View; entryno: INTEGER; OUT snr: INTEGER;
				OUT comment: ARRAY OF CHAR);
	BEGIN
		ASSERT(v IS StdView, 20);
		WITH v: StdView DO
			IF entryno <= v.nentries THEN
				snr := v.history[entryno].snr;
				IF v.history[entryno].comment = NIL THEN comment := ''
				ELSE comment := v.history[entryno].comment$
				END
			END
		END
	END GetMoreData;