FWD » Core Development » Base Language

In OE the following code:

OUTPUT TO out.
  MESSAGE "Inner".
OUTPUT CLOSE.
MESSAGE "Outer".

Emits only Outer.

In FWD, both Inner and Outer messages are emitted.

My questions are:

1. why is this? Is this due to a bug, or to some difference in configuration?
2. in which situations the RedirectedTerminal is really used as a terminal? Can anyone provide me with 4gl example?

Branch 9572a created for this task. The RedirectedTerminal refactored in rev. 15768. No functional changes yet.

As to how to fix the original problem:

At the moment the sizes of terminal forced as 512 characters by 128 lines. If we emit something bigger to the terminal, then we are in trouble.
My proposal is:

- Create a terminal with virtually unlimited size.
- Use StringBuilder to store a terminal line instead of a char[].
- Use a List[StringBuilder] to store lines instead of an array of char[].
- Allocate lines and characters dynamically on demand.

Vladimir, are you testing in GUI 4GL? In ChUI, both MESSAGE show on screen. Please check if there is some configuration/startup parameter which disables MESSAGE when redirected terminal in GUI.

Constantin Asofiei wrote:

Vladimir, are you testing in GUI 4GL? In ChUI, both MESSAGE show on screen.

Yes, I am testing in Swing GUI. And yes, in ChUI more messages are visible.

Please check if there is some configuration/startup parameter which disables MESSAGE when redirected terminal in GUI.

I have not found anything related here:

https://docs.progress.com/bundle/openedge-database-management/page/Startup-Parameters.html

So, I think, this is a bug.

I tested with this demo application, and I am confused a bit:

OUTPUT TO 9572.txt.

DEFINE FRAME f
  "Decimal:   "     SKIP
  "Integer:   "     SKIP
  "Char: "          SKIP
  "Date:      "     SKIP
  "Logical:   "     SKIP
  "Value: "         SKIP
  WITH SIZE 45 BY 31 TITLE "Test 9572" 
  .
OUTPUT CLOSE.
MESSAGE "Outer   ".

The 9572.txt file contents in OE:

                Test 9572
Integer:
Char:
Date:
Logical:
Value:

And in FWD:

Test 9572
Decimal:
Integer:
Char:
Date:
Logical:
Value:

Seems, both are not correct:

1. In OE, the header overwrites the "Decimal: " text, but is indented to be centered on the screen.
2. In FWD, the header does not overwrite the text, but is not aligned.

I wonder, if the header alignment shall be fixed?

There are other issues that require discussion, but I can resolve the origin issue this by allocating enough space to accommodate longer lines.

Please minimize the changes. I don't want to refactor this at this time. I don't want to move to a list approach. Can we just detect the moment we need more space and reallocate the entire set of arrays at a larger size (pick a safe amount)?

So instead of 127 lines by 512 characters, we would have 127 lines by (512 + X) characters where X is some reasonable value that is large enough to hold the data being written.

Greg Shah wrote:

Please minimize the changes. I don't want to refactor this at this time. I don't want to move to a list approach. Can we just detect the moment we need more space and reallocate the entire set of arrays at a larger size (pick a safe amount)?

So instead of 127 lines by 512 characters, we would have 127 lines by (512 + X) characters where X is some reasonable value that is large enough to hold the data being written.

We do not need to make all lines the same length, just allocate enough characters on demand for each write character operation.

Hmmm. I think we have dependencies in that code on the assumption that all rows are the same size. And we have external classes like ScreenBitmap that make the same assumption. I don't see a good reason to move away from that approach. It will be a rare case when we must reallocate and the amount of memory used is not enough to justify making the code more complicated.

Greg Shah wrote:

Hmmm. I think we have dependencies in that code on the assumption that all rows are the same size.

I haven't found any evidence for this in the current implementation of the RedirectedTerminal code.

And we have external classes like ScreenBitmap that make the same assumption.

Yes, but IMO, this is unrelated to the current issue, as the bitmap isn't used within the existing RedirectedTerminal code.

I don't see a good reason to move away from that approach. It will be a rare case when we must reallocate and the amount of memory used is not enough to justify making the code more complicated.

We must significantly increase the buffer size (approximately doubling it) upon each reallocation; otherwise, we'll end up reallocating on every character.

Also, we have the following implementation for clean:

   /**
    * Wipe the slate clean (initialize our state to its pristine condition).
    */
   private void clear()
   {
      Arrays.fill(num, 0);
      Arrays.fill(box, false);

      for (int i = 0; i < lines; i++)
      {
         Arrays.fill(buffer[i], ' ');
      }

      ...

Here we fill the entire terminal area with spaces, even if we change just one character. This can happen frequently (e.g., after every MESSAGE call). I'd prefer to allocate space dynamically as needed to avoid unnecessary operations, and never clear anything past n[cy].

I also need to fix the clearArea. The meaning of the right and bottom arguments is unclear to me: are these indices inclusive or exclusive?

The Javadocs do not provide any hint. However, based on the implementation, it appears that: right is treated as exclusive, and bottom is treated as inclusive.

We must significantly increase the buffer size (approximately doubling it) upon each reallocation; otherwise, we'll end up reallocating on every character.

Doubling seems excessive. Use some fixed "chunk" size like 128 characters and increase it by the number of chunks needed to contain the new larger line.

With this change, we can also start the terminal size smaller since most terminals will fit inside less than 140 characters width.

Greg Shah wrote:

We must significantly increase the buffer size (approximately doubling it) upon each reallocation; otherwise, we'll end up reallocating on every character.

Doubling seems excessive.

Use some fixed "chunk" size like 128 characters and increase it by the number of chunks needed to contain the new larger line.

No objection.

With this change, we can also start the terminal size smaller since most terminals will fit inside less than 140 characters width.

We can start from size 0 easily, and do not pre-allocate anything at all.

Question about getScreenBitmap / setScreenBitmap:

What is the purpose of the bitmap? It does not appear to be used inside RedirectedTerminal, and RedirectedTerminal provides no way to access its contents other than dumping it to a stream.

If the bitmap is not actually used, can we assign arbitrary fixed dimensions? Currently, it is set to 512x128, which already seems arbitrary.

Why didn't you check in the unit tests to 9572a? If they were useful for this task then they should be checked in.

Sorry, you're right. I missed the test files.

You can ask Marian if you need help in finding the right spot for the 4GL code.

Early review 9572a.

In drawVBorder, shouldn't the previous state be copied to the new re-allocated array?

Otherwise awful lot of unrelated and non-functional changes for such a trivial issue. Very hard to spot the effective changes. Vladimir, if you need to do non-functional refactoring changes, can you please at least put them in separate checkins?

Hynek Cihlar wrote:

Early review 9572a.

Otherwise awful lot of unrelated and non-functional changes for such a trivial issue. Very hard to spot the effective changes. Vladimir, if you need to do non-functional refactoring changes, can you please at least put them in separate checkins?

The non-functional refactorings in RedirectedTerminal were made in the previous separate checkin in rev. 15768. Please, see the changes 15768-15796 for the functional changes.

Hynek Cihlar wrote:

Early review 9572a.

In drawVBorder, shouldn't the previous state be copied to the new re-allocated array?

You are right. Just fixed this, will commit the change soon.

I am fixing the moveOutputToColumn(final int fromCol, final int toCol), currently implemented as:

   public void moveOutputToColumn(int fromCol, int toCol)
   {
      int firstRow = this.cy;
      int lastRow  = this.last;
      for (int i = firstRow; i <= lastRow; i++)
      {
         if (num[i] > 0)
         {
            char[] row = Arrays.copyOf(buffer[i], buffer[i].length);

            Arrays.fill(buffer[i], ' ');
            System.arraycopy(row, fromCol, buffer[i], toCol, num[i]);
            num[i] = num[i] - (fromCol - toCol);
         }
      }
   }

From the code, it appears that this method shifts the contents of lines within the range from cy to last. The cy value corresponds to the line affected by the last terminal drawing operation. However, since this method is called from a location that performs no drawing, is it possible that cy could hold an arbitrary value here?

Also, without any assumptions about the arguments, the method does not include protections against overflowing the line boundaries.

The breakable field's lifecycle is unclear. It maintains a set of line numbers that can only grow; the set is never cleared, even after a terminal reset. Is this the expected behavior?

I've rebased 9572a to rev. 16118. Please, review.

Hynek Cihlar wrote:

Code review 9572a.

[...]
The javadoc is misleading, the width is used to allocate the buffer width, default window width, etc.

This value is not used in the RedirectedTerminal code. It can be set from outside and retrieved back. I can update the description if this is misleading. How should it read, by your opinion?

Please rename back wdth parameter of drawHBorder to width.

It clashed with the field name, it this is a small issue. I can rename it back if you think this is necessary.

sync and view methods don't check for buffer != null. Also should they check for i >= buffer.length?

You are correct, there can be situations, when the buffer is accessed in null state. I introduced the getRow(int) method to encapsulate read access to buffer.

ensureCapacity ignores x on initial line allocation (newLine = new char[CHAR_BUFFER_INCREMENT]).

You are correct. Fixed.

length calculation in view doesn't seem right, it is assigned an index of the last non-space char.

You are right. Fixed. Also optimized by using the existing lastNonBlankIdx() methos. Also added the testView() test method to test the view() method.

txt = new String(line, boxOffset, lastNonBlankIdx + 1 - boxOffset); is suspicious, can lastNonBlankId be assigned to -1?

This should be OK after the length calculation is fixed.

ScreenBitmap size will get out of sync with the size of the redirected terminal when it grows its capacity since bitmap field is assigned.

I conclude that the @ScreenBitmap* is determined by the terminal's width and height, which remain unaffected by terminal output.

See the fixes in rev. 16123.

Vladimir Tsichevski wrote:

Hynek Cihlar wrote:

ScreenBitmap size will get out of sync with the size of the redirected terminal when it grows its capacity since bitmap field is assigned.

I conclude that the @ScreenBitmap* is determined by the terminal's width and height, which remain unaffected by terminal output.

Right, the screen bitmap should only be affected by the screen size and not the buffer size.

Hynek Cihlar wrote:

Code review 9572a. Please start regression testing. I suppose I will run the ChUI regression tests.

ChUI regression tests passed.

Hynek Cihlar wrote:

Code review 9572a. Please start regression testing. I suppose I will run the ChUI regression tests.

Could you propose customer scenarios associated with the redirected terminal?

Vladimir Tsichevski wrote:

Hynek Cihlar wrote:

Code review 9572a. Please start regression testing. I suppose I will run the ChUI regression tests.

Could you propose customer scenarios associated with the redirected terminal?

Search for the OUTPUT TO statement in Hotel GUI and the testcases projects, or the other customer apps, if you will find them relevant.

Hynek Cihlar wrote:

Vladimir Tsichevski wrote:

Hynek Cihlar wrote:

Code review 9572a. Please start regression testing. I suppose I will run the ChUI regression tests.

Could you propose customer scenarios associated with the redirected terminal?

Search for the OUTPUT TO statement in Hotel GUI and the testcases projects, or the other customer apps, if you will find them relevant.

I evaluated the Hotel application, including the export HTML operation, and the results were OK. Regarding the testcases, I could not locate a test catalog that builds without encountering multiple conversion and compilation errors. Are you aware of any such test catalogs?

Vladimir Tsichevski wrote:

Hynek Cihlar wrote:

Vladimir Tsichevski wrote:

Hynek Cihlar wrote:

Code review 9572a. Please start regression testing. I suppose I will run the ChUI regression tests.

Could you propose customer scenarios associated with the redirected terminal?

Search for the OUTPUT TO statement in Hotel GUI and the testcases projects, or the other customer apps, if you will find them relevant.

I evaluated the Hotel application, including the export HTML operation, and the results were OK. Regarding the testcases, I could not locate a test catalog that builds without encountering multiple conversion and compilation errors. Are you aware of any such test catalogs?

Try the old testcases project, there are many *redirected*.p test cases.

Vladimir, what is the status of this issue?

Hynek Cihlar wrote:

Vladimir, what is the status of this issue?

You suggested:

Try the old testcases project, there are many redirected.p test cases.

I have not found files matching this pattern in the xfertestcases project (xfer.goldencode.com/opt/testcases/).

Vladimir Tsichevski wrote:

Hynek Cihlar wrote:

Vladimir, what is the status of this issue?

You suggested:

Try the old testcases project, there are many redirected.p test cases.

I have not found files matching this pattern in the xfertestcases project (xfer.goldencode.com/opt/testcases/).

~/secure/code/p2j_repo/testcases/

Hynek Cihlar wrote:

Try the old testcases project, there are many redirected.p test cases.

There are about 170 matching test files - I don't know where to start :-)
It would be good to rewrite them as ABLUnit tests first.

Vladimir Tsichevski wrote:

Hynek Cihlar wrote:

Try the old testcases project, there are many redirected.p test cases.

There are about 170 matching test files - I don't know where to start :-)

Pick some representative subset.