issue-#106 View restored twice on Open

User avatar
DGDanforth
Posts: 1061
Joined: Tue Sep 17, 2013 1:16 am
Location: Palo Alto, California, USA
Contact:

Re: issue-#106 View restored twice on Open

Post by DGDanforth »

Josef,
Operationally it really doesn't matter where the flag is placed since in my
code there is only one place for which it is tested.

Conceptually Views is the correct place,
but as you say, this is a hack anyway.

I would not object to leaving it as Robert posted.

-Doug
User avatar
Robert
Posts: 1024
Joined: Sat Sep 28, 2013 11:04 am
Location: Edinburgh, Scotland

Re: issue-#106 View restored twice on Open

Post by Robert »

DGDanforth wrote:...but as you say, this is a hack anyway.
My original suggestion was a 'hack' with the dual purpose of having Doug test if it solved his problem, and, if so, of allowing him to modify his private installation if we decide not to put a solution into the official build. At that time I did not know how many different sources of the Restore call we needed to consider, so an INTEGER variable (called src) was appropriate. We now understand that only two paths need to be distinguished.

I have suggested four refinements to this hack:
1 - Change the type to BOOLEAN
2 - Change the variable name (Dougs name is even better)
3 - Move the variable to Views
4 - Document it.

Is it still a hack? We might have different opinions. But if it is we should probably either NOT include it, or make further improvements.
Waiting for "somebody with more knowledge ... (to) provide a better solution" seems unrealistic. We are the best experts actively working in this area, and should decide on the solution to adopt now. Of course it is possible that a better solution may be found by a new expert, but that is true of every line in the framework, not just this one. When it is found we will then decide what to do about it.

The current proposal not only "cannot do any harm to existing views", it cannot provide "possible incompatibilities" with a later "solution to the underlying problem"; if indeed there is an underlying problem.

So, for me, treating this as a normal new Feature is a better approach than creating and publishing a new category of hidden, undocumented, "smart-hack".
User avatar
Josef Templ
Posts: 2047
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#106 View restored twice on Open

Post by Josef Templ »

Robert wrote: 4 - Document it.
When you try to document it you will find out that it is not possible to define the special case(s) where the flag can be used.
How does it relate to scroll bars, what about containers, what about views embedded inside another view,
what about other not yet tested cases? This is all based on experiments.

We should try to keep this simple and pragmatic, which to me means undocumented, or in other words as a low-level (Host) feature.
For me, personally, it is not important to have the flag in 1.7.1 and
I certainly don't want to delay the 1.7.1 release further.

- Josef
User avatar
Robert
Posts: 1024
Joined: Sat Sep 28, 2013 11:04 am
Location: Edinburgh, Scotland

Re: issue-#106 View restored twice on Open

Post by Robert »

I have completely changed my mind!
(Not strictly true, as I only said it could be made a permanent feature, not that it should be made one.)

1 - We should reject this issue as no bug has been identified. The current behaviour may be inefficient in some circumstances, but that is not a bug.
2 - The so-called "smart-hack"" should not be included in the master build.
3 - If Doug wants to continue using it in his private installation that is his buisness.

View.Restore can be called in circumstances where HostWindows.src is undefined, so it is not suitable for general distribution.

Just in case anyone wants to investigate (experiment) further my test View is below:

Code: Select all

TYPE
  View  =  POINTER TO RECORD (Views.View) END;

PROCEDURE (v : View) Restore (f : Views.Frame; l, t, r, b : INTEGER);
  BEGIN
    ASSERT (HostWindows.src IN {1, 2}, 45);
    IF  HostWindows.src = 2  THEN  f.DrawRect (l, t, r, b, Ports.fill, Ports.red)  END;
    HostWindows.src  :=  -1
  END Restore;

PROCEDURE (v : View) HandlePropMsg (VAR msg : Properties.Message);
  BEGIN
  WITH  msg : Properties.SizePref  DO
    IF  (msg.w = Views.undefined)  OR  (msg.h = Views.undefined) THEN
      msg.w  :=  50 * Ports.mm;
      msg.h  :=  20 * Ports.mm
    END
    ELSE
    END
  END  HandlePropMsg;
User avatar
Josef Templ
Posts: 2047
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#106 View restored twice on Open

Post by Josef Templ »

Robert wrote: View.Restore can be called in circumstances where HostWindows.src is undefined, so it is not suitable for general distribution.
I don't think that my version suffers from this problem but it is possible that
I have overlooked something. Anyway, for a review I have put it in branch issue-#106.
Download from http://blackboxframework.org/unstable/i ... b1.973.zip.

Note that I have set/reset the flag outside the Kernel.Try(CreateDoc...) statement in order to make it TRAP safe, i.e.
to reset the flag even if a TRAP occurs in Restore.

The diffs are here: https://redmine.blackboxframework.org/p ... 7d44afd75d.

- Josef
User avatar
Robert
Posts: 1024
Joined: Sat Sep 28, 2013 11:04 am
Location: Edinburgh, Scotland

Re: issue-#106 View restored twice on Open

Post by Robert »

I think your name creatingDoc (and my name initialRestore) are simpler to understand, but Doug's name docCreated leads to (marginally) simpler filtering logic in the client's Restore.
Josef Templ wrote:[... i.e. to reset the flag even if a TRAP occurs in Restore.
How does that work?
(This is probably a rhetorical question; I am not expecting a long answer explaining the inner workings of the trap handling mechanism.)
User avatar
Josef Templ
Posts: 2047
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#106 View restored twice on Open

Post by Josef Templ »

Robert wrote:I think your name creatingDoc (and my name initialRestore) are simpler to understand, but Doug's name docCreated leads to (marginally) simpler filtering logic in the client's Restore.
Josef Templ wrote:[... i.e. to reset the flag even if a TRAP occurs in Restore.
How does that work?
(This is probably a rhetorical question; I am not expecting a long answer explaining the inner workings of the trap handling mechanism.)
Well, it depends.
If you want to optimize a simple view's Restore, then it suffices to add a single line at the beginning:

Code: Select all

IF HostWindows.creatingDoc THEN RETURN END;
IMHO, a RETURN is justified here because it keeps the changes in a single line.
If you later remove this optimization (because somebody from oberoncore has solved it:-)
you need to remove a single line only.
If you then look at a textual diff you see only a single line changed, not the full body of Restore.
As you observed, the semantics of the flag is made very clear with this naming.
Anyway, it is left to the user how the flag is used, and the difference is very small.

@TRAP safe:
Kernel.Try catches the trap and then continues with the next statement after Kernel.Try as if it had returned normally.

- Josef
User avatar
Robert
Posts: 1024
Joined: Sat Sep 28, 2013 11:04 am
Location: Edinburgh, Scotland

Re: issue-#106 View restored twice on Open

Post by Robert »

In build 977 the Changes document has a rather long entry for bug 106.
I suggest we change the issue description to remove how to observe the bug, and add a (very short - 1 line) description of the workaround we have implemented.
User avatar
Josef Templ
Posts: 2047
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#106 View restored twice on Open

Post by Josef Templ »

Robert wrote:In build 977 the Changes document has a rather long entry for bug 106.
I suggest we change the issue description to remove how to observe the bug, and add a (very short - 1 line) description of the workaround we have implemented.
Done. See https://redmine.blackboxframework.org/issues/106.

- Josef
Post Reply