bug in StdCoder.Select

[Josef Templ]

StdCoder.Select is a Notifier for the selection box in the Tools->Decode form.
It contains a Dialog.Update(par) call, which confuses the selection box.
The box shows more or less random behavior when selecting/deselecting multiple lines.
The same error exists in BB1.6.

Code: Select all

	PROCEDURE Select*(op, from, to: INTEGER);
		VAR p: FileList; i: INTEGER;
	BEGIN
		IF (op = Dialog.included) OR (op = Dialog.excluded) OR (op = Dialog.set) THEN
			IF NofSelections(par.list) = 1 THEN
				i := 0; p := par.files;
				WHILE ~par.list.In(i) DO INC(i); p := p.next END;
				par.storeAs := p.name
			ELSE par.storeAs := ""
			END;
			Dialog.Update(par)   <=== this call needs to be removed
		END
	END Select;

- Josef

[Josef Templ]

please comment

[luowy]

I've done a test:
1,select 2 or more valid file names in a textview
2,Tools-->Encode File List
3,open the "Decode" Dialog by Inserting a command views at begining of the new document
4,test it !

yes, your fixup worked well.
the Update operation must has been done elsewhere,though I am not clear;

luowy

[Josef Templ]

I also don't understand the details and I am not very happy with this lack of knowledge.
Is it generally forbidden to call Dialog.Update for a control from its own notifier
because of the potential danger of an endless recursion?
Can Dialog.Update be made to work even for this situation?
At what time is an update of which control possible without the need to call Dialog.Update?
All I know is that my fix looks good in tests.

May be the experts from Oberoncore can provide some comments.

- Josef

[luowy]

I check it again,found :

Code: Select all

Dialog.UpdateString(par.storeAs);

worked well as the

Code: Select all

(*Dialog.Update(par);*)

it's weird!

[Josef Templ]

Calling Dialog.UpdateString(par.storeAs) explicitly is an improved work around
because it avoids the dependency on the automatic update of storeAs that is not defined anywhere,
or is it?

The reason why it works is simple:
The Dialog.Update(par) updates ALL interactors contained in the address range covered by par.
This includes storeAs but also list, which is the Dialog.Selection interactor that shows the problem.
By calling Dialog.UpdateString(par.storeAs) the update is narrowed to storeAs only.
With respect to list, it is the same as not calling Dialog.Update at all.

There is a more fundamental flaw in the notifier logic of a selection box.
In a selection box that already has a selection, a Mouse-Left-Click on an unselected line
means a sequence of two operations:
1. deselect (exclude) what is currently selected
2. select (include) the line clicked at
There are multiple notifier calls in this case!
Actually, there is a separate notifier call for every single line that is part of an edit operation.
Whenever there are multiple notifier calls there is a problem with calling
Dialog.Update from the notifier. What happens is that the editing operation is
terminated with the state at the first notifier call. This explains the observed random-like behavior.

- Josef

[Josef Templ]

I found the relevant and buggy code in HostCFrames.HandleCommand/Update
and I also found THE solution (after extensively trying dead ends).
Any Update of a selection box while it is performing a sequence of notifier calls to
sync with an editing operation must be delayed, i.e. there must be an Update after the
editing operation has been finished. This also improves the runtime behavior from quadratic to linear!
Two boolean flags need to be introduced in the SelectionBox:

Code: Select all

		SelectionBox = POINTER TO RECORD (StdCFrames.SelectionBox)
			i: Info;
			num: INTEGER;
			delayUpdate, needsUpdate: BOOLEAN   <== new code required
		END;

Update must be delayed:

Code: Select all

	PROCEDURE (f: SelectionBox) Update;
		VAR res, i, j, a, b: INTEGER; sel: BOOLEAN;
	BEGIN
		IF f.delayUpdate THEN f.needsUpdate := TRUE; RETURN END;  <== new code required
		...

The notifier loop must be protected from Update in HandleCommand for SelectionBox:

Code: Select all

			c.delayUpdate := TRUE; c.needsUpdate := FALSE;  <== new code required
			i := 0;
			WHILE i < c.num DO
				j := i;
				res := WinApi.SendMessageW(c.i.ctrl, WinApi.LB_GETSEL, j, 0);
				IF c.sorted THEN j := WinApi.SendMessageW(c.i.ctrl, WinApi.LB_GETITEMDATA, j, 0) END;
				c.Get(c, j, b);
				IF (res # 0) & ~b THEN c.Incl(c, j, j)
				ELSIF (res = 0) & b THEN c.Excl(c, j, j)
				END;
				INC(i)
			END;
			c.delayUpdate := FALSE;  <== new code required
			IF c.needsUpdate THEN c.Update END  <== new code required

This change is not a workaround as my previous proposal was and also luowy's improvement is.
It is a true fix of the problem that works for any selection box and it does not require the special
rule that a selection box notifier must not call Dialog.Update for its own interactor.

With the solution found I think it is time to file an issue.

- Josef

[luowy]

Two boolean flags need to be introduced in the SelectionBox:

How about one boolean flag?

Code: Select all

SelectionBox = POINTER TO RECORD (StdCFrames.SelectionBox)
			i: Info;
			num: INTEGER;
			(*delayUpdate, needsUpdate: BOOLEAN*)
			updating:BOOLEAN;
		END;

Code: Select all

PROCEDURE (f: SelectionBox) Update;
		VAR res, i, j, a, b: INTEGER; sel: BOOLEAN;
	BEGIN
		(*IF f.delayUpdate THEN f.needsUpdate := TRUE; RETURN END;*)
		IF f.updating THEN RETURN END;

Code: Select all

  
      PROCEDURE HandleCommand (wnd: WinApi.HANDLE; wParam, lParam: INTEGER);

			(*c.delayUpdate := TRUE; c.needsUpdate := FALSE;*)
					c.updating:=TRUE;
					i := 0;
					WHILE i < c.num DO
						j := i;
						res := WinApi.SendMessageW(c.i.ctrl, WinApi.LB_GETSEL, j, 0);
						IF c.sorted THEN j := WinApi.SendMessageW(c.i.ctrl, WinApi.LB_GETITEMDATA, j, 0) END;
						c.Get(c, j, b);
						IF (res # 0) & ~b THEN c.Incl(c, j, j)
						ELSIF (res = 0) & b THEN c.Excl(c, j, j)
						END;
						INC(i)
					END;
					(*c.delayUpdate := FALSE;*)
					(*IF c.needsUpdate THEN c.Update END*)
					c.updating:=FALSE;
					c.Update();
				END

it can worked same as two flag as far as I tested;

The HostCFrames.action do the checked update all the time,except the notifier procedure.
maybe it is the reason which I not understand before: worked after comment out Dialog.Update .

[Josef Templ]

I also considered the version with a single flag but rejected it.
The reason is that I don't want to do ANYTHING automatically, i.e. without calling Dialog.Update explicitly.
This way it is guaranteed that everything behaves exactly as before when Dialog.Update was not called explicitly
(and therefore Update was not called and this is the same now).
The flag doesn't hurt. Actually it doesn't even require extra memory because it fills a gap.

- Josef