FWD » Core Development » Base Language

Make sure you have a testcase that does something like this:

INPUT THROUGH echo $USER NO-ECHO.
SET the-current-user AS CHARACTER.
INPUT CLOSE.
MESSAGE the-current-user.

Is any way to check what display is showing now in 4gl ?

What do you mean by "display"?

For example if I set ECHO I want to know in code if something is displayed and if I set NO-ECHO then display buffer is empty.

OK, I understand what you are trying to do.

I think there are 2 options (test these to see if either or both work):

1. Try redirecting the terminal output to a file:

OUTPUT TO echo_test.log.

2. Try redirecting STDOUT for the 4GL process:

pro 1> echo_test_stdout.log

This is less desirable than the OUTPUT TO because to use the same technique in P2J the user running the testcase must know/remember to execute the client with STDOUT redirected.

From there, you can examine the log file and look for specific entries. Make each line of your INPUT source unique so that you can tell which statements generate output or not.

If you use different filenames for the log statements for each INPUT case, then you could use the 4GL to detect if those files existed and if they have a non-zero size.

Constantin: any other ideas?

I'm responding to the note in #1629.

Am correct ?

Yes. Although, I am surprised it doesn't work (we support other define var cases that are in a format phrase).

Also if I run testcases not from terminal (for example for IDE or other sources) than I got: ** Process launch failed.. (-2), but I think it's normal situation.

I suspect you are right.

Greg Shah wrote:

Yes. Although, I am surprised it doesn't work (we support other define var cases that are in a format phrase).

Do I need to look it now ? or it's not part of this task ?

No. But please do create a new Redmine task (a bug) that describes the problem and has the testcase code for the recreate. Make me a "watcher" on that task. Create the task in this "Base Language" project.

After investigation about problem in display while stream is opened, I've found next problem:
1) if output stream is opened, then in 4gl (and p2j) every display goes to this output stream.
2) after the stream is closed and if no any other display call into this frame, then all displayed data (after stream is opened, but before stream is closed)will not be shown. In P2J this work properly.
3) but if try to display after stream is closed, then all data will shown. I think it's redraw (refresh) all data into frame.

3) but if try to display after stream is closed, then all data will shown. I think it's redraw (refresh) all data into frame.

Please post the example code and output here.

Greg Shah wrote:

3) but if try to display after stream is closed, then all data will shown. I think it's redraw (refresh) all data into frame.

Please post the example code and output here.

testcase:

def var flag as logical.
def var displayFrame as logical.

message "display frame" update displayFrame.

display "before 'input from'".

output to value ("test.txt") append.

put unformatted "~n ".

display "something between".

put unformatted "this is the appended text".

if displayFrame then
   display "some test,".

output close.
message "output close" view-as alert-box.

if displayFrame then
   display "after close".

┌────────────────────────────────────────────────────────────┐
│before 'input from' something between some test, after close│
└────────────────────────────────────────────────────────────┘

┌────────────────────────────────────────────────────────────┐
│before 'input from'                                         │
└────────────────────────────────────────────────────────────┘

┌────────────────────────────────────────────────────────────┐
│before 'input from'                              after close│
└────────────────────────────────────────────────────────────┘

┌────────────────────────────────────────────────────────────┐
│before 'input from'                                         │
└────────────────────────────────────────────────────────────┘

Interesting.

It seems to me that all 4 DISPLAY statements are being output in the same "default" (unnamed) frame.

The first DISPLAY is before redirection occurs. It draws the frame on the screen and draws the first literal text widget. There is space left behind for the other possible literal strings which the layout manager would plan for in the order of definition.

I think that the redirection after the first display causes the strange behavior here. Even though the display "something between". code is unconditional (no if displayFrame then) the output is placed in the frame but it is somehow buffered or the frame is not being refreshed.

That buffered or unviewed content is then "flushed" to the already viewed screen only if there is another DISPLAY (or probably any VIEW) of that frame AFTER redirection is turned off.

If the frame wasn't already displayed when the redirection was started, then this behavior would not occur.

Note the difference in output to the test.txt file between these cases:

displayFrame eq yes case:

before 'input from' something between
this is the appended text

before 'input from' something between some test,

displayFrame eq no case:

before 'input from' something between
this is the appended text

This strange output behavior is due to the flushing that occurs when you mix frame output and stream output to the same destination. Does P2J handle the non-terminal portions OK?

The thing that we don't do today in the terminal portion is that the output done to that frame during the redirected period is not made visible in the interactive frame. I suspect this may have something to do with how we "switch" the screen rendering (see OutputPrimitives and RedirectedTerminal classes) for a special redirected mode renderer. We will have to make special provisions to honor this behavior.

Go ahead and fix this.

Code Review 0502a

1. Is the GenericFrame change intentionally meant to only modify the case where DISPLAY is used with unnamed stream redirection (and not when DISPLAY is used with named streams)?

2. Please provide some more description of your intentions with the changes in GenericFrame.view(FrameElement[]). It looks like you are forcing all widgets in the frame to be displayed in this case. I wonder if there are scenarios where there are widgets in this list which should not be displayed.

3. Does GenericFrame.idsFromWidgets() do the same thing as you are doing in view()? If so, you can reuse that code.

4. In regard to the NEWLINE in DirStream, please test on windev01 to see if it is needed.

Greg Shah wrote:

Code Review 0502a

1. Is the GenericFrame change intentionally meant to only modify the case where DISPLAY is used with unnamed stream redirection (and not when DISPLAY is used with named streams)?

will check it.

2. Please provide some more description of your intentions with the changes in GenericFrame.view(FrameElement[]). It looks like you are forcing all widgets in the frame to be displayed in this case. I wonder if there are scenarios where there are widgets in this list which should not be displayed.

Yes, I'm forcing all widget in frame to "refresh" their state. Best solution is remember only widget which have been displayed after stream open, but it'll be more complicated. I'm trying to find some tests if some widget should be not displayed.

3. Does GenericFrame.idsFromWidgets() do the same thing as you are doing in view()? If so, you can reuse that code.

Yes, I'll fix it.

4. In regard to the NEWLINE in DirStream, please test on windev01 to see if it is needed.

NEWLINE is not needed for windows server

Behaviour of display and stream functionality is more complicated than I thought before.

Yes, it is very tricky. It took us a long time to get it "mostly right". I know it is not completely correct right now, but it is more correct than incorrect.

In regard to your testcase and results, I believe that it it reasonably consistent. I will annotate the code to explain what I think is going on:

def var flag as logical.
def var displayFrame as logical.

/* at this point, there are no named streams and the unnamed stream is equivalent to the terminal */

define stream scr.
output stream scr to terminal.

/* now there is a named stream that also is equivalent to the terminal */
/* output to either the unnamed stream or to scr should be the same */

define stream rep.
output stream rep to FieldSizeReport.txt.

/* now there is another named stream, but it represents a file */

flag = yes.
displayFrame = yes.
message "display frame" update displayFrame.

display "1)first msg".  /* A: this line outputs the default (unnamed) frame to the unnamed stream which is the terminal, at this point only the 1st static text widget is visible in the "screen-buffer" */

output to value ("test.txt") append echo.

/* now the unnamed stream is redirected to a file */

display stream scr "2) second msg". /* B: this line outputs the default (unnamed) frame to the named stream which is the terminal, at this point both the 1st and 2nd static text widgets are visible in the "screen-buffer" */

display "3) third msg". /* C: this line outputs the default (unnamed) frame to the unnamed stream which is the file test.txt, at this point all 3 static text widgets are visible in the "screen-buffer", I guess the output file should have all 3 in it? */

message "test message".
output close.

/* the unnamed stream is now the terminal again */

message "output close" view-as alert-box.

if displayFrame then display stream rep. /* D: this line outputs the default (unnamed) frame to the file FieldSizeReport.txt, no new output to the terminal here but all 3 static text widgets would get output to the file */

/*
if displayFrame then display stream scr. E: this line would output the default (unnamed) frame to the terminal, all 3 static text widgets in the screen-buffer would get output to the terminal
 */

message "end" view-as alert-box.

Are all my notes correct based on your file outputs?

It is important to know that all your display statements are creating a single unnamed (default) frame. This default frame has a single screen-buffer, whose current state is output to whatever destination is specified in the output statement (e.g. DISPLAY in this case). In other words, the screen-buffer exists at a frame level and does not have different versions for each destination.

scrStream.isTerm() return false for scr stream, but I'm not sure should it return true in this situation ?

I would have expected it to return true once the output stream scr to terminal executes. There may be something wrong with the code in StreamFactory.openFileStream() or in our conversion.

Everything is correct, except one:

C: this line outputs the default (unnamed) frame to the unnamed stream which is the file test.txt, at this point all 3 static text widgets are visible in the "screen-buffer", I guess the output file should have all 3 in it?

In the end, all 3 widgets exists in file, but not after this line. (I think after stream is closed). As I know display is not supported unbuffered statement. So I think after close all data is flushed.
File output for this testcase:

1)first msg 2) second msg 3) third msg
test message

The client code will have to be looked at closely in regard to flushing. There are other times when flushing occurs besides closing the stream. For example, when you switch the stream to which you output a frame, it triggers a flush of the stream from which you are switching away.

Code Review 0607a

1. GenericFrame is missing a history entry.

2. Can GenericFrame.refreshWidgets() could use Utils.integerCollectionToPrimitive() instead of the hand-coded approach?

3. The javadoc for GenericFrame.refreshWidgets() seems to be incorrect.

4. Can GenericFrame.cacheWidgetIDs() use Collections.addAll() instead of the hand-coded approach?

5. The GenericFrame.view(FrameElement[] data) use of cacheWidgetIDs() seems like it will never do anything. That method has a short-cut at the top that is executed when UnnamedStreams.output() != null, which means that in that case the cacheWidgetIDs() will not be executed. The cacheWidgetIDs() will only trigger when UnnamedStreams.output() == null but in that case, cacheWidgetIDs() will return without doing anything.

6. There are cases where GenericFrame.refreshWidgets() is called many times during one display() invocation. For example, GenericFrame.display() will trigger refreshWidgets() at least 3 times because refreshWidgets() is invoked directly in that method as well as in both downstream methods that are called: GenericFrame.display(FrameElement[]) and GenericFrame.display(Stream, FrameElement[]) (when unnamed output is redirected). This is confusing at a minimum. The code is protected from actually calling down to the client, but it seems like there are cases where the call is unnecessary (GenericFrame.display() seems not needed ever).

7. It is not clear that the approach is correct. Some questions:

• Why is the caching being done before some calls to LogicalTerminal.view() but not for others?
• The caching of IDs occurs whenever UnnamedStreams.output() != null, but the output that is being done may be targeted at either UnnamedStreams.output() OR some other completely different output destination. It is not clear if all of these cases should trigger the caching. My suspicion is that it should be more limited.
• The refreshWidgets() is done during display statements, but it is not clear why/when it is needed. Should this only be done when the output destination is changing or is there some other limiting factor that comes into play?
• Is the refreshWidgets() only supposed to be done for DISPLAY and not for VIEW statements?

Greg Shah wrote:

Code Review 0607a

1. GenericFrame is missing a history entry.
2. Can GenericFrame.refreshWidgets() could use Utils.integerCollectionToPrimitive() instead of the hand-coded approach?
3. The javadoc for GenericFrame.refreshWidgets() seems to be incorrect.
4. Can GenericFrame.cacheWidgetIDs() use Collections.addAll() instead of the hand-coded approach?

fixed, will provide in next update

5. The GenericFrame.view(FrameElement[] data) use of cacheWidgetIDs() seems like it will never do anything. That method has a short-cut at the top that is executed when UnnamedStreams.output() != null, which means that in that case the cacheWidgetIDs() will not be executed. The cacheWidgetIDs() will only trigger when UnnamedStreams.output() == null but in that case, cacheWidgetIDs() will return without doing anything.

You are right, I've been added code here just for "safe", if this code will changed later.

6. There are cases where GenericFrame.refreshWidgets() is called many times during one display() invocation. For example, GenericFrame.display() will trigger refreshWidgets() at least 3 times because refreshWidgets() is invoked directly in that method as well as in both downstream methods that are called: GenericFrame.display(FrameElement[]) and GenericFrame.display(Stream, FrameElement[]) (when unnamed output is redirected). This is confusing at a minimum. The code is protected from actually calling down to the client, but it seems like there are cases where the call is unnecessary (GenericFrame.display() seems not needed ever).

Yes, it was not always clear where we need to save Ids, I will recheck it again.

7. It is not clear that the approach is correct. Some questions:

• Why is the caching being done before some calls to LogicalTerminal.view() but not for others?

I did not cache only in two places with frameID as widget LogicalTerminal.view(frameId). As I understand all frame cannot be hided by different stream.

• The caching of IDs occurs whenever UnnamedStreams.output() != null, but the output that is being done may be targeted at either UnnamedStreams.output() OR some other completely different output destination. It is not clear if all of these cases should trigger the caching. My suspicion is that it should be more limited.

Don't know at the moment, need research this.

• The refreshWidgets() is done during display statements, but it is not clear why/when it is needed. Should this only be done when the output destination is changing or is there some other limiting factor that comes into play?

Widgets will shown only after stream is close and if any new display command will run. If output destination when nothing happened. see testcases from note-25

• Is the refreshWidgets() only supposed to be done for DISPLAY and not for VIEW statements?

refreshWidgets() supposed to be done only for DISPLAY. VIEW statement doesn't do anything in this situation.

5. The GenericFrame.view(FrameElement[] data) use of cacheWidgetIDs() seems like it will never do anything. That method has a short-cut at the top that is executed when UnnamedStreams.output() != null, which means that in that case the cacheWidgetIDs() will not be executed. The cacheWidgetIDs() will only trigger when UnnamedStreams.output() == null but in that case, cacheWidgetIDs() will return without doing anything.

You are right, I've been added code here just for "safe", if this code will changed later.

Please put a // comment there that explains this (that it should not be executed but is being placed their for safety).

• Why is the caching being done before some calls to LogicalTerminal.view() but not for others?

I did not cache only in two places with frameID as widget LogicalTerminal.view(frameId). As I understand all frame cannot be hided by different stream.

What do you mean by all frame cannot be hided by different stream?

At a minimum, please put // comments in those locations explaining that the cacheWidgetIDs() is not needed there. That way future readers won't think it was mistakenly left out.

• The refreshWidgets() is done during display statements, but it is not clear why/when it is needed. Should this only be done when the output destination is changing or is there some other limiting factor that comes into play?

Widgets will shown only after stream is close and if any new display command will run. If output destination when nothing happened. see testcases from note-25

I don't see how these limitations are coded. There are no checks for these conditions.

• Is the refreshWidgets() only supposed to be done for DISPLAY and not for VIEW statements?

refreshWidgets() supposed to be done only for DISPLAY. VIEW statement doesn't do anything in this situation.

Please add text to the refreshWidgets() javadoc explaining that the 4GL only triggers this behavior from DISPLAY and does not trigger it from VIEW.

I need to determine Terminal stream. But got a problem:

define stream scr.
output stream scr to terminal.

TransactionManager.registerTopLevelFinalizable(scrStream, true);
scrStream.assign(null);

boolean isTerm = stream == null                                 || 
                 stream instanceof StreamWrapper                && 
                 ((StreamWrapper) stream).dereference() == null || 
                 stream.isTerm();  

Greg Shah wrote:

Please check if this is working. Does this resolve your problem? Does this work for the general cases where there is an explicit redirection of the terminal:

Yes it resolved my problem, and now everything are working fine. I've just finishing testing and upload update soon.

Code Review 0616a

I'm OK with the changes. Please run both conversion and runtime regression testing. If you haven't run conversion on devsrv01 recently, you will have to run without your changes to get a fresh baseline for comparison purposes.

This update does change code in a very sensitive area (the GenericFrame changes). I am a bit worried about that part. As such, screen and report failures will have to be examined very carefully.

Constantin: do you have any concerns with my changes (or with Evgeny's)?

Greg Shah wrote:

Constantin: do you have any concerns with my changes (or with Evgeny's)?

The changes look OK to me.

Regression testing is finished. But atm I've found quite a lot timeout failed tests:
50 in gso_tests
42 in tc_tests
I've run tests twice time but most of them was failed by timeout in both tries. Now I'm running regression without update to see difference, maybe I was just unlucky.

Find a testcase that fails regularly and use the test XML file or the results details to see the recreate. Manually run your server and a client (see the timco.html for details on how to do that). Try to recreate the problem manually. If you can do that, then when you are at the problem location you can use F2 to get to help and the current program can be seen in the PROGRAM item on the help screen.

Code Review 0624a

The merge looks fine.

After analyzing converted source code, I've found next difference:
without updates code:
UnnamedStreams.assignOut(null);
with updates code:
UnnamedStreams.assignOut(StreamFactory.openTerminalStream());

Some of logic in GenericFrame class works different after this changes, for example method:
com.goldencode.p2j.ui.GenericFrame#display(com.goldencode.p2j.ui.FrameElement[])
there is code:

 if (out != null) // doesn't change if out is terminal
      {
         display(out, data);
      }
      else
      {
         // set the value in the screen buffer for each widget using the widget
         // id as the index
         copyToScreenBuffer(data);

         // now make the frame visible, bring it to the top and refresh the
         // contents
         view(data);
         refreshWidgets(null);
      }

Maybe problem in wrong conversion or unnecessary stream redirection.
I've looked on text.p source code and didn't find any streams in this code. But in converted code (either without updates):

doBlock(TransactionType.FULL, "blockLabel0", new Block()
               {
                  public void body()
                  {
                     if (_isEqual(EnvironmentOps.getOperatingSystem(), "unix"))
                     {
                        UnnamedStreams.assignOut(StreamFactory.openFileStream("/dev/null", true, false));
                        UnnamedStreams.safeOutput().setEcho(false);
                     }
                     else
                     {
                        if (_isEqual(EnvironmentOps.getOperatingSystem(), "msdos"))
                        {
                           UnnamedStreams.assignOut(StreamFactory.openFileStream("nul", true, false));
                        }
                     }

                     OnPhrase[] onPhrase0 = new OnPhrase[]
                     {
                        new OnPhrase(Condition.ERROR, Action.RETRY, "gText")
                     };

                     doWhile(TransactionType.SUB, "gText", new LogicalExpression(true), onPhrase0, new Block()
                     {
                        public void body()
                        {
                           OnPhrase[] onPhrase1 = new OnPhrase[]
                           {
                              new OnPhrase(Condition.ERROR, Action.RETRY, "blockLabel1")
                           };

                           doBlock(TransactionType.FULL, "blockLabel1", onPhrase1, new Block()
                           {
                              public void body()
                              {
                                 new FindQuery(xyNotes, (String) null, null, "xyNotes.key asc, xyNotes.seq asc", LockType.NONE).last();
                                 if (_isUnknown(xyNotes.getKey()))
                                 {
                                    next("gText");
                                 }
                                 tLastText.assign(xyNotes.getKey());
                                 notes.create();
                                 RecordBuffer.startBatch();
                                 notes.setKey(plus(tLastText, 1));
                                 notes.setSeq(new integer(1));
                                 notes.setFileName(new character("notes-file"));
                                 notes.setFieldName(new character("notes-field"));
                                 RecordBuffer.endBatch();
                                 RecordBuffer.prepare(xyNotes);
                                 new FindQuery(xyNotes, "xyNotes.id = ?", null, "xyNotes.key asc, xyNotes.seq asc", new Object[]
                                 {
                                    notes.recordID()
                                 }, LockType.NONE).unique();
                                 RecordBuffer.prepare(notes);
                                 new FindQuery(notes, "notes.id = ?", null, "notes.key asc, notes.seq asc", new Object[]
                                 {
                                    xyNotes.recordID()
                                 }, LockType.EXCLUSIVE).unique();
                                 leave("gText");
                              }
                           });
                        }
                     });

                     UnnamedStreams.assignOut(StreamFactory.openTerminalStream());
// without update here is UnnamedStreams.assignOut(null);

                     hideMessage(true);

                     notes.setSeq(new integer(0));
                     tKey.assign(notes.getKey());
                     endSeq.assign(0);
                  }
               });

About note 42: what you are seeing are unnamed input streams (which are opened via a OUTPUT TO ... and INPUT FROM ... statements). As you can notice, there is no explicit stream name used. In P2J, these get converted to UnnamedStreams.assignOut and assignIn API calls.

About note 41: you need to check all the methods which receive a Stream parameter from GenericFrame, and all the UnnamedStreams.output() and UnnamedStreams.input() usage. In cases when you have code like stream != null, you need to replace it with !stream.isTerm(); the other case, where stream == null needs to be replaced with stream.isTream().

The ThinClient logic should be OK, because this already is implemented with TerminalStream - only the server-side logic was using null to identify a terminal stream (input or output).

Please post your latest findings, your latest code and your testing results as they stand.

Code Review 0717a

Generally the changes look good to me.

Constantin, what do you think?

Some feedback:

1. In ThinClient.redirectedOutput() there is this code:

      int old = switchWorker(newId);

      // all we need to do is delete it from the map
      if (oldId != -1)
      {
         Stream out = sd.streamFromId(oldId);  <-- this hides the int var named old above

         if (oldId != old && out != null && !out.isTerm())  <-- this compares the int oldId to the Stream old

I think this code needs some slightly different naming to work as expected.

2. In ThinClient.switchWorker(), how about changing the code on line 12983 from this:

      if (newId == -1)

to this:

      if (newId == -1 || sd.streamFromId(newId).isTerm())

and then removing the changes to the else block below it.

The only question in my mind is if redirections.containsKey(key) would be true when sd.streamFromId(newId).isTerm() is true, then the behavior would be different than how you have coded it.

The advantage of this approach is it is less code and it is more clear.

3. Please put () around the out != null && out.isTerm() change in UnnamedStreams (line 498). That will make the precedence more explicit and thus easier to read.

Greg Shah wrote:

Constantin, what do you think?

The changes look OK, except for StreamFactory.openFileStream, which still needs to treat the case when the filename is term or terminal - in this case, a TerminalStream needs to be opened. This code should be re-added at the start of this method:

      boolean terminal = false;

      if (filename != null)
      {
         String term = filename.toLowerCase();

         if (term.equals("terminal") || term.equals("term"))
         {
            term = "terminal";
            terminal = true;
         }
      }

      // terminal stream without paging is not allowed
      if (terminal && !paged && pageSz == NO_PAGING)
         return openTerminalStream();

I had removed that code when I changed progress.g to match the strings "term" or "terminal" as the KW_TERM. I prefer to keep that "feature" for the conversion and have any non-legacy code changed to explicitly open the terminal stream instead of indirectly doing it through a special name as a fake file.

Greg Shah wrote:

I had removed that code when I changed progress.g to match the strings "term" or "terminal" as the KW_TERM. I prefer to keep that "feature" for the conversion and have any non-legacy code changed to explicitly open the terminal stream instead of indirectly doing it through a special name as a fake file.

What about the case when the filename is determined at runtime, i.e.:

def stream rpt.
def var ch as char.
ch = "term".
output stream rpt to value(ch).
display stream rpt ch.
output stream rpt close.

This will still output to terminal, and not to file...

Very good point. OK, I guess it is back in.

Evgeny: please make this change and the final changes to finish the update (history entries...). Post that for a final review and it will need one last regression testing run.

ok

Code Review 0721a

1. In StreamFactory.openFileStream() the call to int id = work.obtain().rsb.openFileStream(filename, write, append); must be moved to after the call to:

      {
         return openTerminalStream();
      }

      <--- move code here

Otherwise, in this "terminal" case an actual file named "term" or "terminal" will be opened.

2. The extra call to Stream out = sd.streamFromId(newId); at the top of ThinClient.switchWorker() is not needed.

Greg, I think we should double-check the redir*.p tests from testcases/uast. More, the ThinClient.currentStream assumes that -1 is always the interactive stream. But, if a terminal stream is opened (with its own ID), then a currentStream == streamId test will not produce correct result, when both streams are terminal.

Another question: wouldn't it be better to let the client-side be aware of only one terminal stream? Better said, are we sure it is OK to have multiple client-side terminal streams, each one of them with its own buffer and flush management?

wouldn't it be better to let the client-side be aware of only one terminal stream? Better said, are we sure it is OK to have multiple client-side terminal streams, each one of them with its own buffer and flush management?

This is a very good point and I think it is a problem. The 4GL almost certainly only has 1 terminal instance.

One of the most important places where this is an issue is StreamDaemon.openTerminalStream() which always creates a new instance. I think we will need to track the open instance when it exists and return that same instance multiple times, until it is no longer valid.

On the server-side a -1 stream id is invalid. We have code in both StreamFactory and in RemoteStream that uses the concept that valid IDs are always positive. However, I think those are the only dependent places and the logic is not too complex.

ThinClient.currentStream assumes that -1 is always the interactive stream. But, if a terminal stream is opened (with its own ID), then a currentStream == streamId test will not produce correct result, when both streams are terminal.

You're right. I think there are enough other places where we do depend on that concept that it makes sense to avoid trying to make ThinClient safe for the terminal stream ID to be something else.

My idea is to allow -1 to always be used to refer to the current terminal stream, even on the server. If it is not currently open, then we can open a new terminal steam but its ID would always be -1. Most of the ThinClient changes can be eliminated in that case, right? Of course, the server side will need changes to handle this, but since we don't expose the IDs out to the business logic, this should be reasonable.

Greg Shah wrote:

My idea is to allow -1 to always be used to refer to the current terminal stream, even on the server. If it is not currently open, then we can open a new terminal steam but its ID would always be -1. Most of the ThinClient changes can be eliminated in that case, right? Of course, the server side will need changes to handle this, but since we don't expose the IDs out to the business logic, this should be reasonable.

Do I need to start implement it ?

Greg, looking at the logic where TerminalStream was previously opened by StreamDaemon, I think it was only this case: create a TerminalStream instance only if the stream is paged or a page-size is set. See this code (current P2J) in StreamDaemon:

   public int openFileStream(String filename, boolean write, boolean append)
   throws ErrorConditionException
   {
      if (filename != null && filename.equalsIgnoreCase("terminal"))
         return store(new TerminalStream());

      // terminal stream without paging is not allowed
      if (terminal && !paged && pageSz == NO_PAGING)
         return null;

1. a case when a stream is directed at the terminal. In current P2J, this case is treated via the TerminalStream
2. a case when the terminal is accessed directly. In current P2J, this case is treated via -1 by ThinClient.currentStream, GenericFrame.getRemoteStreamId and maybe others.

The deeper I dig, the more I think that if we merge the TerminalStream usage with the direct terminal, we will break something. I can't recall the case why TerminalStream was added, but there is too much logic which relies on the fact that an explicit stream is in use or not for it to be a coincidence that we decided to use TerminalStream only for the specific case when the stream is paged or page-size is explicitly set.

The deeper I dig, the more I think that if we merge the TerminalStream usage with the direct terminal, we will break something. I can't recall the case why TerminalStream was added, but there is too much logic which relies on the fact that an explicit stream is in use or not for it to be a coincidence that we decided to use TerminalStream only for the specific case when the stream is paged or page-size is explicitly set.

Please look back at notes 21 and 22 above. The issue we are dealing with is the case where the terminal has been specifically opened as a named stream. This technique is most often used when the unnamed stream has been redirected to a file and someone came along later needing to output to the screen so they access it as a named stream and intermix the code with the use of the redirected unnamed stream. It is a stupid thing to do, but the 4GL enables it.

As the testcase in those notes demonstrates, the same frame and its current output state can be output to different destinations. How do we resolve this "named stream as a terminal" usage in a way that is clean?

• in 4GL, application code uses statements to write or read data to/from streams
• these streams can be the default stream (we named these the unnamed streams) or a named stream
• the streams can write or read the data to/from certain devices (terminal, file, process).
• initially, the unnamed streams are associated with the terminal device.
• application logic can decide to associate the unnamed streams with another device, while another named stream is associated with the terminal.
• 4GL has special processing when the output stream is paged, especially when the device is the terminal. To implement this specific case, P2J uses the TerminalStream class.

Currently P2J doesn't have default streams - the terminal is accessed directly. More, when the i.e. unnamed output stream is redirected, the terminal's output is automatically sent to that device, and access to the terminal (via other streams) is no longer possible. This can also be read as, instead of allowing multiple streams to access the terminal device, once the unnamed streams are redirected, P2J hides the terminal device and makes it unavailable for other streams. To get past this limitation, P2J uses a switching mechanism - each time an output statement is executed, it checks first if the stmt targets a stream different than the one currently in use; if so, it temporarily switches to that stream, and restores it while work is done. This stream switching feature is generic enough to allow going back to the terminal, even if the unnamed stream is redirected.

Thus: what part from the case in note 21/22 is not working? I've tested with latest P2J and the code looks fine.

what part from the case in note 21/22 is not working? I've tested with latest P2J and the code looks fine.

Evgeny: this question is for you.

def var ch1 as char init "1".
def var ch2 as char init "2".
def var i as int.
form ch1 ch2 with frame f1.

message "view frame?" update vf as log.
if vf then view frame f1. /* force frame to be already visible */

message "down in stream?" update isdown as log.

def stream rpt.
output stream rpt to test.txt.

pause message "ch1".
display stream rpt ch1 with frame f1.
if isdown then down stream rpt with frame f1. /* clear the screen buffer for the frame */

pause message "ch2".
output stream rpt close.

display ch2 with frame f1.

pause message "done".

• vf = no, isdown = no
  4GL:
  

  ┌─────────────────┐
  │ch1      ch2     │
  │──────── ────────│
  │1        2       │
  └─────────────────┘
  

  
  P2J:
  

  ┌─────────────────┐
  │ch1      ch2     │
  │──────── ────────│
  │1        2       │
  └─────────────────┘
  

• vf = yes, isdown = no
  4GL:
  

  ┌─────────────────┐
  │ch1      ch2     │
  │──────── ────────│
  │1        2       │
  └─────────────────┘
  

  
  P2J:
  

  ┌─────────────────┐
  │ch1      ch2     │
  │──────── ────────│
  │         2       │
  └─────────────────┘
  

• vf = no, isdown = yes
  4GL:
  

  ┌─────────────────┐
  │ch1      ch2     │
  │──────── ────────│
  │         2       │
  └─────────────────┘
  

  
  P2J:
  

  ┌─────────────────┐
  │ch1      ch2     │
  │──────── ────────│
  │         2       │
  └─────────────────┘
  

• vf = yes, isdown = yes
  4GL:
  

  ┌─────────────────┐
  │ch1      ch2     │
  │──────── ────────│
  │         2       │
  └─────────────────┘
  

  
  P2J:
  

  ┌─────────────────┐
  │ch1      ch2     │
  │──────── ────────│
  │         2       │
  └─────────────────┘
  

P2J works incorrect in the case when the frame is already visible, and a widget (which is already displayed) has a different value than the one displayed on screen (the internal screen buffer has changed for that widget). This is case (vf = yes, isdown = no) from above. I think what 4GL does is that, when a widget is displayed, it refreshes all the widgets in the frame, with their latest values from the frame's screen buffer. This behaviour looks like is limited to DISPLAY/UPDATE/PROMPT-FOR/ENABLE - VIEW does not exhibit this. This problem is distinct enough to be worked after the main #1628 issue (or as part of another task).

• The progress.g change which emits the StreamFactory.openTerminalStream can remain - but conversion rules need to be enhanced to cover all this cases:
  

  def stream rpt.
  
  output stream rpt to term.
  output stream rpt to term paged.
  output stream rpt to term paged page-size 10.
  output stream rpt to term page-size 10.
  
  output to term.
  output to term paged.
  output to term paged page-size 10.
  output to term page-size 10.
  
  output stream rpt to term append.
  output stream rpt to term append paged.
  output stream rpt to term append paged page-size 10.
  output stream rpt to term append page-size 10.
  
  output to term append.
  output to term append paged.
  output to term append paged page-size 10.
  output to term append page-size 10.
  
  input stream rpt from term.
  input from term.
  

  
  and to return a null stream if the terminal is not paged and page-size is not set. Note that not all of these convert properly, with the current update - parameters are not properly set and no distinction is made between write and read modes; new StreamFactory.openTerminalStreams APIs need to be added, to accept parameters as the openFileStream API does. Also, the StreamBuilder.openTerminalStream needs to be changed, to accept the write parameter - append I think it can be ignored.
• backout the changes which assume a TerminalStream is in use, when the stream's target device is actually the physical terminal (no paged/page-size set, the old "currentstream=-1" or "stream=nul" cases need to be added back) and:
  - StreamFactory.openFileStream needs to return null if the physical terminal is targeted.
  - StreamFactory.openTerminalStream needs to return null if the physical terminal is targeted (as openFileStream does)
  - StreamDaemon.openFileStream needs to return a TerminalStream instance, if the filename is the term or terminal string.
  - GenericFrame - if the stream is null, then no redirection is in place (the physical terminal is targeted). The !out.isTerm needs to be removed - is enough to assume that if a stream is in place, then the terminal device is not targeted directly
  - the Stream.isTerm API: I think this API is confusing and should be removed. In latest P2J, no class overrides this, so it always returns false. The only case which was made to return true is the TerminalStream implementation - but this is not the physical terminal either. isTerm should return true only if an actual Stream implementation targets the physical terminal directly - but P2J has no such case.
  - ThinClient - same, if stream ID is -1, then the physical terminal is targeted.

P2J works incorrect in the case when the frame is already visible, and a widget (which is already displayed) has a different value than the one displayed on screen (the internal screen buffer has changed for that widget). This is case (vf = yes, isdown = no) from above. I think what 4GL does is that, when a widget is displayed, it refreshes all the widgets in the frame, with their latest values from the frame's screen buffer. This behaviour looks like is limited to DISPLAY/UPDATE/PROMPT-FOR/ENABLE - VIEW does not exhibit this. This problem is distinct enough to be worked after the main #1628 issue (or as part of another task).

Good work in isolating the issue. I agree that this should be handled in a separate task. I will create that and we will work it later.

For this task, the next steps are:
...

Evgeny: please follow this plan. Ask questions as needed.

Greg Shah wrote:

Evgeny: please follow this plan. Ask questions as needed.

• For this task I need to rollback changes which contains things related to #2345?
• Provide update for final review with changes for this task ?

For this task I need to rollback changes which contains things related to #2345?

Yes, following the implementation guidance documented by Constantin in note 63.

Provide update for final review with changes for this task ?

Yes.

Code Review 0728a

I am fine with the changes.

Evgeny: please merge 0728a.zip with the latest bzr revision - #2352 has changes which need to be merged.

Evgeny, about 0731a.zip: the StreamFactory.openTerminalStream(boolean, int, boolean) needs to return NULL only if the stream is not paged and its page-size is not set, i.e. if ( !paged && pageSz == NO_PAGING) { return null; }. In all other cases, (stream paged or page size is set), a real TerminalStream needs to be constructed.
More, the conversion of the openTerminalStream APIs doesn't seem correct. Check this case: output to term. is converted to openTerminalStream(false), but the signature of this API is boolean paged - I think the correct signature of this API is boolean append.

After you make these changes, go ahead and get it conversion and runtime tested.

Evgeny Kiselev wrote:

Do I need to specify in com.goldencode.p2j.util.StreamDaemon#openTerminalStream page size ?

No, I don't think so. In the previous logic, StreamDaemon.openFileStream doesn't pass the page/page-size to the new TerminalStream instance.

The changes look good. Go ahead and regression test them (conversion and runtime).

Code Review 0805a

I agree that an alternate approach in StreamWrapper seems more correct.

StreamWrapper line 404:
before it was always true inOk = true; and outOk = true;, maybe more correct:
inOk = stream null || stream.isIn(); and outOk = stream null || stream.isOut();

If you make this change, please format it like this:

   inOk  = (stream == null || stream.isIn());
   outOk = (stream == null || stream.isOut());

Have you tested this approach?

I edited the previous note to fix the logic I had broken...

In StreamWrapper with next code:

   inOk  = (stream == null || stream.isIn());
   outOk = (stream == null || stream.isOut());

   inOk  = true;
   outOk = true;

OK, then we need to leave the code as:

   inOk  = true;
   outOk = true;

Please document (in comments inside that method) that you have tried the alternate approach and how it failed. Then post the final code here.

Tests are failed too, but just few of them and didn't repeat too often. And also ctrlc part failed.

We need to get this through testing. Are there any tests which are always failing?

Greg Shah wrote:

We need to get this through testing. Are there any tests which are always failing?

CTRL-C part:
ctrlc_11_session1 step 18-28
ctrlc_11_session3 step 9-23, 25
ctrlc_11_session4 step 1-26, 28

Main part:
gso_430
gso_276
tc_job_002
tc_job_clock_002

tc_job_002
tc_job_clock_002

Are these the known failures as described in our documentation?

The other problems definitely need to be investigated. Please provide more details of your findings.

CTRL-C part:
ctrlc_11_session1 step 18-28
ctrlc_11_session3 step 9-23, 25
ctrlc_11_session4 step 1-26, 28

Failed with error:
Timeout while waiting for event semaphore to be posted.

Two tests failed with:
Timeout before the specific screen buffer became available (Mismatched data at line 1, column 0. Expected '┌' (0x250C at relative Y 1, relative X 0) and found 'e' (0x0065 at relative Y 1, relative X 0).)

Differents:
Should be "┌─────" (top left corner border of window), but found:

Killed2014evk@devsrv01:~$ ^Cion: TIPR833K  TIMCO - GREENSBORO                         ───┐
evk@devsrv01:

I've tried to run this tests manually and they worked fine.

main part:
gso_276 step 33
Wrong result Record(s) selected: 167 ...... 25 Pages, but should be Record(s) selected: 167 ...... 24 Pages (checked manually confirm this issue)

Next tests were failed time to time:
gso_269 step 48
gso_430 step 19
gso_290 step 19
gso_167 step 35
gso_293 step 38

Evgeny Kiselev wrote:

CTRL-C part:
ctrlc_11_session1 step 18-28
ctrlc_11_session3 step 9-23, 25
ctrlc_11_session4 step 1-26, 28

Failed with error:

These are not of concern, they are usual "false negatives".

main part:
gso_276 step 33
Wrong result Record(s) selected: 167 ...... 25 Pages, but should be Record(s) selected: 167 ...... 24 Pages (checked manually confirm this issue)

Next tests were failed time to time:
gso_269 step 48
gso_430 step 19
gso_290 step 19
gso_167 step 35
gso_293 step 38

Did this pass in a previous run?

Also When we removed isTerm() method, there are next possible issues:
RemoteStream had constructor RemoteStream(int id, boolean term) and term = true could be only in one situation in com.goldencode.p2j.util.StreamFactory.openFileStream(java.lang.String, boolean, boolean, int, boolean) when filename is "terminal" and stream is paged or page size specified.

Also in com.goldencode.p2j.ui.GenericFrame.isOutTerm() could be true only when stream.isTerm() = true, after refactoring we've lose this functionality.
Same problem in com.goldencode.p2j.util.KeyReader.readKey(com.goldencode.p2j.util.Stream, double) we've lose some functionality.

Evgeny Kiselev wrote:

Also When we removed isTerm() method, there are next possible issues:
RemoteStream had constructor RemoteStream(int id, boolean term) and term = true could be only in one situation in com.goldencode.p2j.util.StreamFactory.openFileStream(java.lang.String, boolean, boolean, int, boolean) when filename is "terminal" and stream is paged or page size specified.

You are correct, isTerm is overriden by RemoteStream and code on server-side is dependent on this. Please add back these.

Code Review 0812a

The proposed code can be accepted with only some minor changes (listed below).

However, I think the problems 10/11 in #1629 note 17 will NOT be fixed since you have undone the changes in GenericFrame and ThinClient. Am I correct? These problems are related to NO-ECHO support, so they do need to get fixed. I would be OK with getting the current changes through testing and checked in, before fixing the last problems. But we must fix those before we can close out this task.

In regard to the two samples listed in note 86: does the P2J behavior match the 4GL with your 0812a update? In other words, these problems are with the changes needed to fix the problems 10/11 in #1629 note 17?

Cleanup for 0812a:

1. At this point the StreamWrapper changes are only in formatting and comments. The history entries are no longer valid. Please just change my history entry to "Minor cleanup of formatting and comments." and remove your history entry.

2. The removal of && !paged && pageSz == NO_PAGING from StreamFactory seems dangerous. Please provide more information on why you made this change. I also wonder if part of our problem is that this code:

         if (term.equals("terminal") || term.equals("term"))
         {
            term = "terminal";
            terminal = true;
         }

should probably be this:

         if (term.equals("terminal") || term.equals("term"))
         {
            filename = "terminal";   <--- this code was setting term but it was supposed to set filename
            terminal = true;
         }

problem described in note 86 divide into 2 problems:
1) incorrect position for message method with redirected stream (I think I've fixed this problem, now tests are running)
2) problem with put unformatted "~n ".:
at the moment this code converted into:

UnnamedStreams.safeOutput().putUnformatted(new FieldEntry[]
            {
               new PutField("\n ")
            });

problem with put unformatted "~n ".:
...
and in method com.goldencode.p2j.util.Stream#generateFormattedText applies format for this string

This doesn't seem correct. How does Stream.putUnformatted(FieldEntry[]) call Stream.generateFormattedText()? putUnformatted() calls putWorker() and generateFormattedText() is only called by two of the putField() methods.

Here is my stacktrace:

main@1, prio=5, in group 'main', status: 'RUNNING'
      at com.goldencode.p2j.util.Stream.generateFormattedText(Stream.java:5984)
      at com.goldencode.p2j.util.Stream.putField(Stream.java:3437)
      at com.goldencode.p2j.util.PutField.output(PutField.java:946)
      at com.goldencode.p2j.util.Stream.putWorker(Stream.java:5176)
      at com.goldencode.p2j.util.StreamDaemon.putWorker(StreamDaemon.java:1005)

OK, that makes more sense now. What do you suggest?

Evgeny Kiselev wrote:

Before merge this update have been passed regression testing, but now it not compile because of ChuiScreenDriver class.

It was a problem in rev 10602, 10603 is OK now.

Code Review 0820a

I am fine with the changes.

However, I think the problems 10/11 in #1629 note 17 will NOT be fixed since you have undone the changes in GenericFrame and ThinClient. Am I correct? These problems are related to NO-ECHO support, so they do need to get fixed. I would be OK with getting the current changes through testing and checked in, before fixing the last problems. But we must fix those before we can close out this task.

I still don't know the answer to this question from note 87.

And I would like to know your suggested approach to solving the PUT UNFORMATTED newline issue.

Are there any other open issues to resolve?

Greg Shah wrote:

I still don't know the answer to this question from note 87.

And I would like to know your suggested approach to solving the PUT UNFORMATTED newline issue.

Are there any other open issues to resolve?

Yes, this is last one issue.

I know, that 4gl documentation is not always correct, but there is next paragraph in put statement:

UNFORMATTED
Tells the AVM to display each expression in the same format produced by the
EXPORT statement, but without quotes.

In out code, we add extra new line only for MODE_EXPORT, but in this situation we need to add extra line only when we have some other output. Also if I fix problem with formatting, it didn't calculate \n symbols while calculating number of characters in particular line.

In out code, we add extra new line only for MODE_EXPORT, but in this situation we need to add extra line only when we have some other output.

Are you referring to the use of endOutput(true) in Stream.putWorker()?

Also if I fix problem with formatting,

What is your proposed formatting fix?

it didn't calculate \n symbols while calculating number of characters in particular line.

What code are you referring to here?

Code Review 0910a

I am worried that your changes are just there to fix some very specific behavior in a few testcases, but that the fixes are not general purpose. I expect these changes to cause serious problems.

1. Why is lazyTrailingNL only set true in putField(BaseDataType val, String fmt)? There are many other paths through variants of putField() that will bypass this. This doesn't seem right. Also, if you do call writeNewLine(), then we may also need to call rawIncrementNextLineNum(). But more importantly, I don't see why adding this lazyTrailingNL is correct at all. What you are describing is just that PUT UNFORMATTED should output its contents without any consideration of newline processing, right? Perhaps we just have PUT UNFORMATTED implemented wrong.

2. Your TerminalStream version is not merged with the latest version in bzr.

3. The RedirectedTerminal change seems dangerous and incorrect. box[i] will only be true when the BasePrimitives.box() method is called. This can happen for any frame that is output where it is NOT configured with the NO-BOX option. Of course, for most frames it is true, but I still don't see why you are making the change. In addition, the redirected output to streams doesn't change for file streams versus other streams, as far as I know. For example:

output through "cat > bogus.txt".
display "hello".
output close.

and

output to "bogus.txt".
display "hello".
output close.

Both of these generate a bogus.txt file that looks like this:

hello

4. You are missing history entries and some places have { that should start on the next line.

What is the status of your regression testing?

What standalone testcases have you run to confirm your changes are correct? I think you need to run the redirected_*.p tests in uast/testcases/.

Greg Shah wrote:

Code Review 0910a

I am worried that your changes are just there to fix some very specific behavior in a few testcases, but that the fixes are not general purpose. I expect these changes to cause serious problems.

1. Why is lazyTrailingNL only set true in putField(BaseDataType val, String fmt)? There are many other paths through variants of putField() that will bypass this. This doesn't seem right. Also, if you do call writeNewLine(), then we may also need to call rawIncrementNextLineNum(). But more importantly, I don't see why adding this lazyTrailingNL is correct at all. What you are describing is just that PUT UNFORMATTED should output its contents without any consideration of newline processing, right? Perhaps we just have PUT UNFORMATTED implemented wrong.

No, this is two different problems
1)

What you are describing is just that PUT UNFORMATTED should output its contents without any consideration of newline processing, right?

yes, this is first problem, but easy to fix.
2) I will try to explain second problem in the next examples.

output to value ("test1.txt") append.
put unformatted "unformat". /*just common string without any special symbols*/
message "message:test".
put unformatted "this is the appended text".
output close.

unformat
message:test
this is the appended text

unformatmessage:test
this is the appended text

I'm trying to explain that 4gl adds new line after each put unformatted if next output is message or display(or maybe good to say that new line added before message or display if previous output was put unformatted). But this new line is not added just after put, it added only before next output. For example last line didn't have new line. Also between two or more put unformatted new line was not added. That's why I've added lazyTrailingNL

3. The RedirectedTerminal change seems dangerous and incorrect. box[i] will only be true when the BasePrimitives.box() method is called. This can happen for any frame that is output where it is NOT configured with the NO-BOX option. Of course, for most frames it is true, but I still don't see why you are making the change. In addition, the redirected output to streams doesn't change for file streams versus other streams, as far as I know. For example:

In my testcase box[] array has been filled just after first display method occur and next message become shifted in 1 space. I'll try to find better decision.

Greg Shah wrote:

What is the status of your regression testing?

I've just started regression testing with latest update.

What standalone testcases have you run to confirm your changes are correct?

yes.

I think you need to run the redirected_*.p tests in uast/testcases/.

Ok

Evgeny Kiselev wrote:

What standalone testcases have you run to confirm your changes are correct?

I think Greg was asking the name of the testcases you used to discover/fix/test your changes. Please commit these to bzr, so I can understand your reasoning/findings behind your changes.

committed testcases into testcases/uast/no_echo
revision 1199.

testcases:
uast/no_echo/put_unformatted_bug.p
uast/no_echo/put_unformtted_bug1.p

Evgeny Kiselev wrote:

testcases:
uast/no_echo/put_unformatted_bug.p
uast/no_echo/put_unformtted_bug1.p

1. please check these issues with a normal PUT. I think what you found applies to it too.
2. there is the SKIP clause for the PUT statement, which allows you to advance to the next row. Check what happens if the PUT ends with a SKIP or SKIP(1).
3. about MESSAGE (and maybe other statements, like DISPLAY) when output is not the terminal: a possibility can be that these statements are checking the cursor position, and automatically start a new line if the cursor is not on the very first column. I think this is confirmed by the following test, which uses SEEK to reposition to a previous position in the file:
   

   output to test1.txt.
   put "this is a very long text" skip 
       "this is line 2 of this text" skip 
       "this is line 3 of this text".
   seek output to 4.
   message "this message is on a new line".
   output close.
   

   
   the output is this:
   

   this
   this message is on a new line
   ne 2 of this text
   this is line 3 of this text
   

   
   without SEEK output is this:
   

   this is a very long text
   this is line 2 of this text
   this is line 3 of this text
   this message is on a new line
   

4. about the box issue:
   - is this a regression from your update or a bug you found?
   - if is a bug you found, please confirm the bug exists with rev 10588 (before the ScreenBitmap changes were introduced)
   - do you have a specific test scenario to duplicate this issue?

Constantin Asofiei wrote:

1. about the box issue:
   - is this a regression from your update or a bug you found?
   - if is a bug you found, please confirm the bug exists with rev 10588 (before the ScreenBitmap changes were introduced)

Yes this bug exists in rev 10588

Constantin Asofiei wrote:

1. about MESSAGE (and maybe other statements, like DISPLAY) when output is not the terminal: a possibility can be that these statements are checking the cursor position, and automatically start a new line if the cursor is not on the very first column. I think this is confirmed by the following test, which uses SEEK to reposition to a previous position in the file:
   

   output to test1.txt.
   put "this is a very long text" skip 
       "this is line 2 of this text" skip 
       "this is line 3 of this text".
   seek output to 4.
   message "this message is on a new line".
   output close.
   

   
   the output is this:
   

   this
   this message is on a new line
   ne 2 of this text
   this is line 3 of this text
   

P2J output:

thisthis message is on a new line
ine 2 of this text
this is line 3 of this text

P2J output with my patch:

this
this message is on a new line
ne 2 of this text
this is line 3 of this text

without SEEK output is this:

this is a very long text
this is line 2 of this text
this is line 3 of this text
this message is on a new line

P2J output:

this is a very long text
this is line 2 of this text
this is line 3 of this textthis message is on a new line

this is a very long text
this is line 2 of this text
this is line 3 of this text
this message is on a new line

Evgeny Kiselev wrote:

P2J output with my patch:

If you are referring to 0910a.zip, try to break it like this:

output to test1.txt.
put "this is a very long text" skip 
    "this is line 2 of this text" skip 
    "this is line 3 of this text".
message "something bogus". /* just so last stmt is not PUT */
seek output to 4.
message "this message is on a new line".
output close.

Constantin Asofiei wrote:

Evgeny Kiselev wrote:

P2J output with my patch:

If you are referring to 0910a.zip, try to break it like this:
[...]
If the last stmt which wrote to the stream is not PUT, then I expect your 0910a.zip to fail.

This testcase is working too for 0910a.zip:
4gl result:

thisthis message is on a new line
ine 2 of this text
this is line 3 of this text
something bogus

thisthis message is on a new line
ine 2 of this text
this is line 3 of this textsomething bogus

thisthis message is on a new line
ine 2 of this text
this is line 3 of this text
something bogus

Evgeny Kiselev wrote:

3. The RedirectedTerminal change seems dangerous and incorrect. box[i] will only be true when the BasePrimitives.box() method is called. This can happen for any frame that is output where it is NOT configured with the NO-BOX option. Of course, for most frames it is true, but I still don't see why you are making the change. In addition, the redirected output to streams doesn't change for file streams versus other streams, as far as I know. For example:

In my testcase box[] array has been filled just after first display method occur and next message become shifted in 1 space. I'll try to find better decision.

The box problem is in fact related to something else: the frame's height (in redirected mode) is computed wrong. It adds a unit for the top border (which is OK) and another unit for the bottom border; and the MESSAGE is actually displayed on this "bottom border" line (thus box[i] is true). But I don't think 4GL adds a new-line after a displayed frame; this case:

def var ch as char.

ch = "this is a test".

form ch with frame f1.

output to a.txt.
display ch with frame f1.
message "this is a message".
output close.

ch
--------
this is

Greg: do you think that adding a row at the bottom of the frame (when output to stream) might be something specific to 4GL 9.1? Or is a defect in P2J?

do you think that adding a row at the bottom of the frame (when output to stream) might be something specific to 4GL 9.1?

It is possible but I think it is very unlikely. Such a change would potentially break many (most?) working applications that depend upon the redirected terminal processing for their reports.

Or is a defect in P2J?

This is much more likely.

Constantin Asofiei wrote:

update 0917

for TerminalStream class you've rollback next lines:

worker.cursorAt(0, 0);
worker.sync();

Evgeny Kiselev wrote:

Constantin Asofiei wrote:

update 0917

for TerminalStream class you've rollback next lines:
[...]
This lines was deleted in 10602 revision. Does these lines needed ?

Please remove them, this was a mistake from my part - I thought these were from your code.

I've found problem in calculating border for frame.
Border is calculating correct, but message will appear on the "border", for example (testcase from not 110):
1) we have the frame:

ch
--------
this is

                         0 row  top border
ch                       1 row
--------                 2 row 
this is                  3 row
this is a message        4 row  bottom border

In the redirected terminal output (see RedirectedTerminal.sync()), we are not supposed to generate output when:

            // output a new line for every touched line except for a
            // touched last line that has no data and had box drawing

In other words, the bottom border normally does not get output when a frame is drawn with BOX on the redirected terminal.

The message statement just outputs at the current row in the redirected terminal. You have already found that we needed to reset the X coordinate to the 0 position. That makes sense.

It seems to me that the "current row" may be wrong when we draw. It may be below the border instead of on the same line as the border.

What is the status of your work on this task?

Code Review evk_upd20141003a.zip

1. The redirected terminal change now looks much safer.

2. Please describe the testing that you did that proves that only the putField(BaseDataType, String) should have lazyTrailingNL enabled. There are many other putField(), put(), putControl(), putNull(), putLineEnd(), putSpace() and putUnformatted() methods which do not flow through this path. Should they be protected? I suspect most or all of these should be protected. If you need to protect all PUT calls, then you should find the workers:

• putWorker() handles all output for put(), putControl() and putUnformatted() variants
• putField(BaseDataType, String, boolean, int) handles all the other putField() variants
• putSpace(int) handles putSpace() too
• putNull(int) handles putNull() too
• putLineEnd(int) handles putLineEnd() too

So there are 5 possible places you might have to change to cover all possible PUT variations. Please provide specific details on how you tested each of these cases and what protection they do or do not need.

3. Please describe the testing you have done to check if DISPLAY, VIEW, MESSAGE and similar redirected terminal usage need the lazyTrailingNL support.

4. Please check in your testcases for the PUT and EXPORT lazyTrailingNL testing.

5. Please look at how writeNewLine() is used in endOutput() and processLineEnd(). When you use it, don't you also need to call rawIncrementNextLineNum()?

6. In StreamFactory and StreamDaemon, please merge the 2 history entries (one by me and one by you) into a single history entry. You can put your initials there.

7. Did you test the uast/testcases/redirected_*.p tests as I requested in note 100? What are the results of that testing?

I EXPECT YOU TO RESPOND IN DETAIL TO ALL MY QUESTIONS ABOVE. Several of these questions I have noted weeks and months ago, but you have never responded.

From Evgeny:

-------- Original Message --------
Subject: Re: Status Report
Date: Thu, 20 Nov 2014 02:28:03 +0300
From: Evgeny Kiselev <evk@goldencode.com>
To: ges@goldencode.com
CC: Faulhaber, Eric <ecf@goldencode.com>, evgeni.kiselev@gmail.com

Hello Greg,

I still working on answers for note 119 from #1628. Most of question I have answer and as soon as I'll have second question I'll post results.
But then I rerun tests from uast/testcases/redirected_*.p, some of tests became not working because of conversion:

redirected_report6_3.p is not converted properly
redirected_report6_4.p is not converted properly
redirected_report6_5.p is not converted properly
redirected_report6_6.p is not converted properly
redirected_report6_7.p is not converted properly

I remembered they were work properly recently. I think it cause then widget ids became dynamic.

Also I received Nullpointer (I think it's the same problem):

Caused by: java.lang.NullPointerException
at com.goldencode.p2j.ui.client.WindowManager.findFocus(WindowManager.java:132)
at com.goldencode.p2j.ui.client.event.EventManager.eventFromKey(EventManager.java:332)
at com.goldencode.p2j.ui.client.TypeAhead.dequeueEvent(TypeAhead.java:419)
at com.goldencode.p2j.ui.client.TypeAhead.getKeystroke(TypeAhead.java:312)
at com.goldencode.p2j.ui.chui.ThinClient.waitForEvent(ThinClient.java:11107)
at com.goldencode.p2j.ui.chui.ThinClient.pause(ThinClient.java:6467)
at com.goldencode.p2j.ui.chui.ThinClient.pause(ThinClient.java:6364)
at com.goldencode.p2j.ui.chui.ThinClient.pauseBeforeHide(ThinClient.java:2838)
at com.goldencode.p2j.ui.client.Window.message(Window.java:1880)
at com.goldencode.p2j.ui.client.Window.message(Window.java:1972)
at com.goldencode.p2j.ui.chui.ThinClient.message(ThinClient.java:6026)

This problems are on latest unmodified revision.

On 11/18/2014 05:27 PM, Greg Shah wrote:

Evgeny,

I'am preparing answers for note 119 from #1628. Am right ?

Yes.

Thanks,
Greg
On 11/17/2014 10:05 PM, Evgeny Kiselev wrote:

Hello,

I believe that we talk about #1628.

At the moment I've merged with latest code, and retesting.
I have answers for most of the questions, but didn't have prove for some of them.

I'am preparing answers for note 119 from #1628. Am right ?

Regards,
Evgeny.

Please provide more information about the conversion problem(s) that occurred.

I would be surprised if the conversion problem was the same as the NPE caused by WindowManager.findFocus(). The NPE is in code that is only executed at runtime. It should not come into play during conversion.

For the NPE, WindowManager.topWindow() must return null which means that no window has ever been added to the window list. This occurs in Window.pushConfig(), Window.getDefaultWindow() and in TitledWindow.show(). This means that you have found a path through our code where the default window initialization has not occurred. Please post the 4GL code here (or point to the testcase/uast/ file) that can be used to recreate the problem.

At the moment regression tests finished with failed status.
5 tests are failed because of different numbers of rows (always difference is 1 in one tests is 2, e.g. 186 rows, but need 185).
So some case in this tests are not cover in my code.

I want to specified again algorithm of lazy NL.
- NL should be added after any PUT (formatted/unformatted) with open unnamed output stream and if next output is message.
or
- Message should be always on New Line if current position is not in the start of line. I think it's more correct. But when we work with Window class or with RedirectedTerminal we have lost all information about data (we don't know if it was message or any other data). And then I try to use com.goldencode.p2j.util.Stream#writeControlled I can add some NL symbol with different type of data (not message).
Conclusion:
- We need know about Stream position in Window or RedirectedTerminal to add needed NL symbol and handle case, that message should start from start of line(I think it's not very good).
- Stream should know about data, we need special flag or special method which should handle message case.

I think your idea of changing the approach seems to be on the right track.

Message should be always on New Line if current position is not in the start of line. I think it's more correct.

I agree, this seems to be a better way to think about the problem.

I've asked Constantin to review this and provide some thoughts.

Evgeny Kiselev wrote:

- Message should be always on New Line if current position is not in the start of line. I think it's more correct. But when we work with Window class or with RedirectedTerminal we have lost all information about data (we don't know if it was message or any other data). And then I try to use com.goldencode.p2j.util.Stream#writeControlled I can add some NL symbol with different type of data (not message).

I think this is the correct approach. MESSAGE should output a new line as needed. Have you determined what happens if the current line contains only white space and the stream cursor is not on the first column? Does the MESSAGE output bellow this white-space line or the cursor is just repositioned on the first column? Use a boxed frame to test (as a NL it's always output after it, as I recall) or a PUT statement ending with SKIP(1) SPACE(<number of spaces>).

- We need know about Stream position in Window or RedirectedTerminal to add needed NL symbol and handle case, that message should start from start of line(I think it's not very good).

Why do you think is not very good? I agree that Stream might need some changes so that it knows when the cursor is on a new line; also, this will isolate this problem to the one who needs, the MESSAGE stmt in this case.

Please merge your update with the latest bzr and post it here.

Constantin Asofiei wrote:

Evgeny Kiselev wrote:

- Message should be always on New Line if current position is not in the start of line. I think it's more correct. But when we work with Window class or with RedirectedTerminal we have lost all information about data (we don't know if it was message or any other data). And then I try to use com.goldencode.p2j.util.Stream#writeControlled I can add some NL symbol with different type of data (not message).

I think this is the correct approach. MESSAGE should output a new line as needed. Have you determined what happens if the current line contains only white space and the stream cursor is not on the first column? Does the MESSAGE output bellow this white-space line or the cursor is just repositioned on the first column? Use a boxed frame to test (as a NL it's always output after it, as I recall) or a PUT statement ending with SKIP(1) SPACE(<number of spaces>).

I was a little bit wrong, see next testcase:

output to value ("message.txt").
put unformatted "unformat~n". 
message "message:test1".
put unformatted " ".
message "message:test2".
put unformatted SKIP(1) SPACE(10).
message "message:test3".
put unformatted "this is the appended text".
output close.

P2J output:

unformat
NL
message:test1
' '
message:test2
NL
'          '
message:test3
this is the appended text

Updated:
We calculate column in Stream properly.

put unformatted "unformat~n".

Please post the corresponding 4GL output so that I can better understand what this code is supposed to do.

Greg Shah wrote:

Please post the corresponding 4GL output so that I can better understand what this code is supposed to do.

Sorry I did mistake, it was 4GL output:

unformat
NL
message:test1
' '
message:test2
NL
'          '
message:test3
this is the appended text

Here is P2J output from latest revision.(Note that in latest revision there is no message:test1)

unformat ' 'message:test2
NL
'          'message:test3
this is the appended text

So we need always NL before message if message is not on the first line(or we need to calculate NL character as not 0 column).

I think instead of "first line" you mean "first column".

Please look closely at ThinClient.sendToStream():

   public int sendToStream(String message)
   {
      int redir;
      Stream out = streamFromId(currentStream);

      // if the terminal is redirected, then send this message to the
      // stream
      if (out != null && out.isUnnamed())
      {
         // assumes that we are in a reset state
         ((ChuiOutputManager) tk).append(message, Color.DEFAULT);
         flushStream(out);
      }

      // the message is displayed on the terminal only if the input is not
      // read from a null device, like /dev/null

      // switch back to interactive mode temporarily, to allow the message
      // to be displayed on the terminal
      redir = switchOutput(-1, false);
      return redir;
   }

I this case, the output is redirected and the destination is unnamed. In other words you used output to value(). which is an unnamed stream instead of output stream my-stream to value(). which is a "named" stream (in this case named "my-stream").

That means that the code executes this:

      {
         // assumes that we are in a reset state
         ((ChuiOutputManager) tk).append(message, Color.DEFAULT);
         flushStream(out);
      }

Note this "assumes that we are in a reset state". I guess this explains why message is behaving badly in this case.

Is the answer to this problem, just to give the stream a chance to flush BEFORE the append()? If there is nothing buffered, then it should do nothing. In other words, if it is already "reset", then it would not flush but instead just return.

ThinClient.sendToStream() is only used for MESSAGE, so any such change seems safe.

Also it's not clear for me, why it was double NL after message:test2 ?

SKIP (with no expression) is a kind of conditional newline. If there is text on the line before the SKIP, then there is a newline added, otherwise it is ignored.

It recall that SKIP(1) is an unconditional newline, but before it puts in the newline it will implicitly execute the SKIP. The use of SKIP does the same thing in interactive frame layout too. In other words, SKIP(1) outputs two newlines in this case, even though it is counter-intuitive.

Code Review evk_upd20150119a.zip

It looks good.

Are the ChUI-specific changes to Window.message() still needed (to set the cursor position to column 0)? If so, please put in a TODO that the ChUI specific code should be moved into the ChUI subclass.

Code Review evk_upd20150121a.zip

The changes are fine. In the future, please post the update without the 01_ prefix.

About the test failures:

For gso_276 and gso_17, you will have to debug in to see the causes.

But I can guess the cause of the gso_488 was caused by the removal of format string processing from the Stream.generateFormattedText().

Does reference results have been taken from 4GL output ?

If this is a report, then it definitely is taken from the 4GL. Almost all screens are taken from the 4GL too, but there are some exceptions.

Eugenie Lyzenko: is there anything about GSO 488 which was not captured in the 4GL?

You can always cut down the line of code that is generating the report detail and create a simple 4GL testcase from it. Then test that on lindev01.

Eugenie Lyzenko: is there anything about GSO 488 which was not captured in the 4GL?

I think yes. The 4GL system dragonfly access was stopped at 04/23/2010, right? The GSO 488 was introduced at 08/30/2010. This is later. However we certainly know how the 4GL reports should be for this test(especially *.csv file).

My changes in Window class is issue for gso_276 test. Looking for approach.

Code Review evk_upd20150126a.zip

1. Please remove import org.apache.commons.logging.*; from Stream.java.

2. The change to Stream.generateFormattedText() does not seem quite right.

• The approach you are taking only will work for character types. Other format strings will never match. For example, decimal default format of ->>,>>9.99 or logical default format of yes/no.
• Use BaseDataType.defaultFormatString() to obtain the real, system-specific default format string for a type. For example, the format for decimal will have the locale-specific group and decimal separator characters.
• Even the character type seems incorrect as the default format for character is x(8). It is not based on the length of the contained text.

Evgeny Kiselev wrote:

My changes in Window class is issue for gso_276 test. Looking for approach.

Did you intend to post a version of Window? Nothing was included.

Code Review evk_upd20150205a.zip

The changes look fine. You'll need to merge up to the latest level before the next set of tests.

This issue depends on ChuiOutputManager.cursorInvalid and RedirectedTerminal.reset variables. If I'm rollback changes in Window class (this changes actually do nothing at the moment, because for RT class cursor position is always 0:0) But this code changes state of cursorInvalid in ChuiOutputManager and for example I'll always lost first message because of code in ChuiOutputManager.append(). I've tried different approaches but nothing helped. I'm always losing NL or received wrong output.

I don't know the answer here. But I am thinking about the following approach:

1. Which changes are causing this? I suspect that only the Window, ThinClient and Stream can have anything to do with this. Even the Stream change is now probably not the cause because it really just should change whether some output is formatted or not AND it really should not be visible in any the MAJIC tests or we would have seen the need for it already. On devsrv01, create a fresh P2J check out and apply only the ThinClient and Window changes. Then bring up a server and manually run the GSO 276 test (using the gso_276.xml, manually enter the inputs as a user). Compare the generated report and confirm that the problem exists with only those changes involved. Then remove the Window change, rebuild and run the test again. Can the problem be isolated to only the ThinClient change?

2. Once you isolate the change that is the cause, you can try a debugging session. Download the MAJIC sources, jars and run/ directories to your local system. Set the database URL to point to your tunnel for your devsrv01 database instance. Using this, you can debug MAJIC. You now can set a conditional breakpoint that triggers when you get to the failing record. Step through the output code on the client side and watch the state to try to figure out where things go wrong.

Constantin: do you have any better ideas?

I think I'm tried to fix wrong bug. The root of my problem is some other bug in latest revision code. At the moment I'm focus on this problem.
Here is testcase:

output to value ("message.txt").
put unformatted "unformat~n". 
message "message:test1".
put unformatted " ".
message "message:test2".
put unformatted SKIP(1) SPACE(10).
message "message:test3".
put unformatted "this is the appended text".
output close.

output to value ("display.txt").
put unformatted "unformat~n". 
display "message:test".
put unformatted "this is the appended text".
output close.

output to value ("export.txt").
put unformatted "unformat~n". 
export "message:test".
put unformatted "this is the appended text".
output close.

unformat

message:test1

message:test2

message:test3
this is the appended text

unformat  message:test2

          message:test3
this is the appended text

Code Review evk_upd20150330a.zip

This seems risky. There are a large number of possible changes this will cause, depending on how many callers depend on that code to return false. My concern is that I can't predict all the ways we use that code and I suspect that we don't want the cursor to be considered valid when it is past the end of the screen.

I've asked Constantin to comment as well.

Also: please only post and/or distribute updates as evk_upd20150330a.zip instead of 01_evk_upd20150330a.zip.

Evgeny, I think a better test is using just a MESSAGE stmt and an unnamed stream, like this:

output to value ("message.txt").
message "message:test1".
output close.

What I think we need is to move the cursorInvalid field from the ChuiOutputManager class to the BasePrimitives class, as this is set only after we call BasePrimitives.cursorAt or BasePrimitives.cursorStay - thus, when the output is switched, the flag state is inherited from the previous terminal, which is incorrect. We need to allow the cursorInvalid flag to be tracked with the output destination in use, and not just use a global flag.

Evgeny Kiselev wrote:

merged with latest revision
moved cursorInvalid to the BasePrimitives
But I couldn't test it now (after merged 29.04.2015 hc_upd20150428b.zip) I got null pointer in testcase from note 141
[...]
I've read all emails but didn't find any special things that I should to do. I do not have any changes in widgets.

I've tested with latest rev (10847) and the note 141 test works fine for me (no abends). All files are OK except display.txt which is missing an empty line.

With AspectJ being introduced, ensure that you are either using the proper p2j.jar or your IDE has AspectJ integrated.

Thx, I've fixed it. now it's working fine for if I worked from p2j.jar instead of using code.

Code Review evk_upd20150504a.zip

1. import org.apache.commons.logging.*; should not be added to Stream.java.

2. Please merge with bzr 10848.

At this point only GSO 276 still fails? All other issues are resolved?

Good, this sounds promising. Please do implement the changes so that they are nicely separated (e.g. no ChUI/GUI-specific stuff in common UI code). Post the result. Hopefully it can get through regression testing and checked in.

Created task branch 1628a from trunk rev 10985.

Rev 10986 contains the merged evk_upd20150713a.zip (there are still some javadocs missing, will fix in the final review/fix).

Runtime testing will start.

Rebased trunk rev from 1628a rev 10986.

1628a rev 10988 fixes a PUT issues (do not emit NL before PUT if cursor is on column 0).

Runtime testing started again.

Code Review Task Branch 1628a Revision 10988

Both the merge of the evk_upd20150713a.zip into 1628a and the changes in 10988 are good.

1628a rev 10989 fixes two more issues (format for unknown date variables and a NL emitted before the page headers).

Branch 1628a passed runtime testing and was merged to trunk rev 10987 and archived.