#23 Exceptions for screen I/O

[Edward Hart]

Here is a patch which detects and raises EC-SCREEN exceptions, as requested here in the discussion of [bugs:#130]. This patch does not include EC-SCREEN-FIELD-OVERLAP.

I'm leaving the patch here for now so that people can check I haven't deleted anything important. In particular, the following from get_line_column():

:::diff

-   if (fcol == NULL) {
-       if (fline->size == 4) {
-           l = p / 100;
-           c = p % 100;
-       } else {
-           l = p / 1000;
-           c = p % 1000;
-       }
-   } else {
-       // ...
-   }

I couldn't make any sense of this. It means that  "DISPLAY foo AT LINE n"  will display foo at column n instead. Was this intended?

[Simon Sobisch]

I did not check the patch yet, but please upload a new one with the removed part included and adding comments. This source likely handles

::cobolfree
    DISPLAY something AT scr-pos END-DISPLAY

if scr-pos is PIC 9(04) (no matter which USAGE is given) then it's evaluated as

::cobolfree
    01 scr-pos.
       05  scr-line pic 9(02).
       05  scr-col  pic 9(02).

otherwise (has cobc/libcob a check for scr-pos having a smaller size than 4?) it's evaluated as

::cobolfree
    01 scr-pos.
       05  scr-line pic 9(n).  *> n is >=2 here
       05  scr-col  pic 9(03).

I've thought about a new folder tests/manual and adding something like scr_001_ec-start_col.cob (all cob files would be automatically generated and run like the test suite but the user would have to check if it works or not (the cob files therefore would need to have a DISPLAY 'This is going to happen next...' and maybe an ACCEPT OMITTED to check if the user presses F1 (not working) or F5 (works).
What are your thoughts about this (I'd add the makefile to make this happen).

Simon

[Edward Hart]

Here is the second version of the patch. It includes the following changes:

• the compiler now checks that in statement like DISPLAY str AT pos that pos is a four or six digit integer
• the implementations of ACCEPT and DISPLAY have been split into two functions: one taking line and column numbers and one taking a single four/six digit integer.

I've thought about a new folder tests/manual ... What are your thoughts about this

I like the idea very much. Any testing is better then none. The one problem is how we'll get people to do them. Will it be part of make check or something we'll have to remember do when coding?

[Edward Hart]

Bump.

[Simon Sobisch]

I'll try to check this next week.

To your question:
Manual tests won't be part of make check as this has to be non-interactive (automated builds do this, too). But I'll come up with something like 'make manualcheck' (maybe too long). Can you upload some of the manual tests? cbl's with DISPLAY 'this is what should happen', then do whatever needs to be checked and maybe an ACCEPT 'pass/fail'.

Simon

[Edward Hart]

Sure, I'll see if I can hack something together over the weekend.

[Brian Tiffin]

I responded as a new entry so people see it, but replying here as well.

We should look at DejaGNU and add a 'make expect' target.

Cheers,
Brian

[Edward Hart]

Here's the first version of the manual screen section testsuite. This version only checks some of the screen section-specific clauses. It can be compiled with "autom4te --language=autotest -o screen_test ./screen_test.at". As I can't get screens to work in the original terminal window, each test executes the test program in a new terminal window.

The first results on my system on xterm are:

• BACKGROUND-COLOR, FOREGROUND-COLOR, REVERSE-VIDEO and UNDERLINE work.
• AUTO fails (when the field marked AUTO is full, the screen session terminates).
• BEEP fails, but only because Ubuntu disables the system beep by default to keep people sane and I have no idea how to enable it.
• FULL fails (the screen session can be terminated without FULL fields being filled).
• HIGHLIGHT and LOWLIGHT fail, but I don't know if xterm support these anyway.
• LEFTLINE and OVERLINE fail
• SECURE fails (with no data entered, the field is already filled with asterisks).
• REQUIRED fails (you can terminate the screen without entering any data in REQUIRED fields)

Any suggestions on how to deal with varied system/terminal support for screen section features?

[Simon Sobisch]

@Ed: please open a bug report for the failed SCREEN IO tests.

BTW: We have an option to use COB_BELL=FLASH, this leads to every BELL leading to flash the screen for reasons like not working system beeps.

[Edward Hart]

Here's an updated patch against [r598].

[Edward Hart]

Another update, this time against [r612]. Any comments?

[Simon Sobisch]

Comments:

1. please don't use

::c
    *line = fline == NULL ? 0 : max_int (0, cob_get_int (fline) - 1);

(both the define max_int and the short evaluation) I know it works but the readability for many programmers I know is very decreased compared to the longer version.

1. Good work on the exceptions. Please take care to reduce the calls to getmaxyx(), this is called quite often. If you haven't check: please verify that invalid move() does not return a specific error code (resulting in the exception-checks checking the return code of move() and not checking them beforehand.) Instead of doing cob_addch within a for loop (and a check for overflow there - resulting checking quite often) check it before entering the loop.

2. Please remove cob_screen_display_at and differentiate via NULL as before (leading to less exported functions).

Do you have any testsuite entries actually checking the new exceptions?

[Edward Hart]

don't use ... the define max_int

Do you mean the use of the ? operator in max_int, rather than the function itself?

take care to reduce the calls to getmaxyx()

I can't do much about that. I would use SIGWINCH (which occurs when the terminal size is changed) but it's not supported on Windows and it's not part of POSIX.

Moreover, getmaxyx() merely expands to something like

:::c
(maxy = ((stdscr)?((stdscr)->_maxy + 1):(-1)), maxx = ((stdscr)?((stdscr)->_maxx + 1):(-1)));

which isn't very taxing and, I think, we can tolerate calling it repeatedly/redundantly as it reduces complexity in the calling functions.

Please remove cob_screen_display_at and differentiate via NULL as before (leading to less exported functions).

What effect does the number of exported functions have?

[Simon Sobisch]

Sorry for the late reply.

Do you mean the use of the ? operator in max_int, rather than the function itself?

I meant two things:

1. Please don't use the short form var = (something) : 0 : a for if (something) {var = 0} else {var = a} - it's not wrong and works with all compilers but the I try to don't use it in GC at all.
2. Please don't use the max_int function but the content of it.

Thank you.

[...] reduce calls to getmaxyx()

This is used in raise_ec_on_truncation() which is called in cob_addch(). cob_addch() is called multiple times in cob_screen_puts() and field_accept():

::c
        for (size = 0; size < f->size; size++, p++) {
            cob_addch ()
            ...
        }
        for (count = 0; count < size_accept; count++) {
            cob_addch ()
            ...
        }

therefore the overflow check is done for each byte of each field. I understand your thought "we can tolerate calling it repeatedly/redundantly as it reduces complexity in the calling functions", but please check if it's possible to do the overflow check only once for the field, not for each byte of it.

What effect does the number of exported functions have?

Not a big effect. The only thing that I consider here is the difference and as it only is

::c
    if (line == NULL && column == NULL) {
        pos_to_line_column (pos, &line, &column);
    } else {
        get_accept_line_and_col_vals (line, column, &sline, &scolumn);
    }

I'd like to stay with the current not-splitted variant (in contrast to adding new declarations).

Simon

[Edward Hart]

Assuming nobody has any comments, I will commit this when the repo is back online.

[Brian Tiffin]

Autotools has hooks for DejaGNU style testing. It uses Tcl based expect, so it's capable of running screenio oriented tests, well, any interactive program really.

It'll be a small bag of work to integrate, especially when it comes to cleanly separating DejaGNU from autotest (they both want to "own" make check) but it might be a worthwhile thing, for current, and for future.

It might be better to not have autotools try and automagically integrate DejaGNU, but instead, hard code some new targets in Makefile.am that allow for make expect (good word for the test kicker, imho)

Cheers

[Brian Tiffin]

Just did a minimal amount of refresher reading. Not only will DejaGNU be handy for testing screenio changes, if we put in a shared Hercules instance, I'm pretty sure DejaGNU will be capable of unit testing code on remote miniframe TSO 3270 screens, and JES2 jobs as well.

Thinking even more, DejaGNU should be capable of testing all of the features we turn up as part of the new Allura install. Will dive deeper. This could be good for a lot of things.

Using the GnuCOBOL Seed integrations, (or Seed by itself, but it'll be more fun if GnuCOBOL is involved), it should be feasible to crank up a webkit browser instance with unit and DejaGNU testing of any web site feature we host.

http://opencobol.add1tocobol.com/gnucobol/#seed

Excuse the drift, but

Time for some experiments...

First up, as to the topic at hand, will be DejaGNU tests for extended screen operations, and unit tests for screenio.c changes, then on to the other pieces.

Cheers

[Edward Hart]

This new patch includes the following changes:

• The function max_int has been deleted
• The functions cob_addch_no_trunc_check and cob_addnch have been added
• The split versions of screen_accept, screen_display, etc. have been removed and all those functions now use the new extract_line_and_col_vals function.

[Simon Sobisch]

Hi Edward. The patch is fine. Please check the minor things below and commit it.

• Why did you moved the timeout to front in the parameters from cob_screen_accept (cob_screen *s, cob_field *line, cob_field *column, cob_field *ftimeout) in the new function screen_accept (s, ftimeout, sline, scolumn) ?
• Please don't use cob_move_cursor in cob_exit_screen as we don't want to raise an exception there. It could be useful to do move (max_y,0) there (I guess this is what should happen), what do you think about this?
• Is there any reason you put #if 0 /* RXWRXW */ } #endif back in typeck.c?
• Please use the snipped below in the testsuite

+AT_CHECK([$COMPILE -prog.cob], [0], [], [])
+AT_CHECK([./prog], [0], [], [])

Thank you very much!

Is there anything you planned to do next? I did not checked the results you've posted before but it looks like FULL and REQUIRED need a fix.

Simon

[Edward Hart]

Why did you moved the timeout...

I can't remember and can't find any way to justify it. I've made parameter order consistent between the two sets of functions.

Please don't use cob_move_cursor in cob_exit_screen ... It could be useful to do move (max_y,0) there (I guess this is what should happen), what do you think about this?

That's an excellent idea. Currently the code assumes all terminals have 24 lines. I've introduced cob_move_to_beg_of_line to implement your suggestion.

Is there any reason you put #if 0 / RXWRXW / } #endif back in typeck.c?

Emacs couldn't indent code properly without it.

The real question is, what is all the RXWRXW stuff for? These snippets are peppered throughout the code and I have no idea what they did and why they're still in the code.

Please use the snipped below in the testsuite

I've changed the first line, but I can't change the second because, despite exceptions being raised, stuff is still sent to stdout. This is what is output when the test is run:

+                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                [1@   


[Simon Sobisch]

I've made parameter order consistent between the two sets of functions.

Good.

I've introduced cob_move_to_beg_of_line to implement your suggestion.

Good - please test this with different terminal sizes before committing.

what is all the RXWRXW stuff for?

These are snippets from Roger While for stuff that was either a planned option (--> these parts may could become active) or stuff that was taken out for testing purposes (--> these parts will be removed if we're sure the current code is better).
You're invited to investigate the RXWRXW for source you already understand quite well :-)

I've changed the first line, but I can't change the second because, despite exceptions being raised, stuff is still sent to stdout.

You're right. Please use AT_CHECK([./prog], [0], ignore, []) instead.

You're free to commit your changes and close this ticket.
Thank you very much for your work on this!

Is there anything you planned to do next? I did not checked the results you've posted before but it looks like FULL and REQUIRED need a fix.

Please create a bug ticket for these if you can confirm your previous results from the semi-automated-test manually.It would be nice if you can investigate/fix these afterwards.

Simon

BTW: For testing the BELL feature with an OS that disables bells you could try to export COB_BELL=SPEAKER or export COB_BELL=FLASH. The internal bell routine is called on wrong input in extended ACCEPT, too; therefore it would be good to configure it in a way it works. I do wonder if configure should set a different value than BELL depending on the OS, at least for OS that are known for blocking this.

[Edward Hart]

Committed in [r635].

I'll report those SCREEN SECTION bugs immediately.

[Simon Sobisch]

The change in [r636] to mark the screen-attributes-not-yet-supported-in-runtime as PENDING. Please check if the testsuite for the necessary tweak - would be either missing compiler test(s) it's even a good idea to test something that is PENDING as this tests the compiler part to recognize/parse this correct] or the need for changed test results.

Please do the same check for the nice split of "Integer value expected" to "Positive integer value expected" "Unsigned integer value expected" - where the only necessary change/lookup is "Positive integer value expected" (the other message belongs to report_integer and Ron changed both the parsing and compiler messages; added lots of testsuite entries).

Please reword the new "AT values must be an must have 6 digits or less than 4 digits.", while I guess I know what you wanted to say others won't understand this.

[Edward Hart]

Please reword the new "AT values must be an must have 6 digits or less than 4 digits."

What did you have in mind? Suggestions:

• "Numeric literal must have 6 digits or less than 4 digits"
• "AT values may not have 5 digits" along with "AT values may not be longer than 6 digits"
• The above, but with "numeric literal" instead of "AT values".

[Simon Sobisch]

Not that easy. But splitting the message in two likely helps people to understand what you meant. What about

values in AT clause may not have 5 digits
values in AT clause may not be longer than 6 digits

You may find a nice wording when checking the syntax rules in the standard and/or MF docs.