FWD » Core Development » User Interface

We have had multiple customers interested in a better browse. The idea is that many customers can accept a version that is not 100% compatible, but does the equivalent job plus some improvements. It would still be limited to the same space as the current browse, although we might want to allow it to layout differently (to better use space if available). This last part would be tricky unless the other widgets were modified too.

We are going to be exploring requirements and will document them here.

This is not specific to the web client. The idea is to implement this in common code so it can be used with ABL screens in either Swing or the web.

The enhancement would be selected at runtime using configuration in the directory.

One idea is to provide generic export to the following formats:

• csv
• xls/xlsx
• pdf

A customer has asked for cell-level hyperlinking. They want to be able to make the cells in a given column clickable and when they are clicked to execute some logic. For example, a phone number column might trigger a call to the listed phone. My idea is to allow you to add a browse column option HYPERLINK "event-name". We would make the associated column's cells a clickable link that when clicked will implicitly PUBLISH "event-name" and pass the cell's current value as the parameter.

Automatically and sensibly expand to fit more space available when the frame and window are sized to allow it.

Improve the look of the browse. Perhaps some of this might be via options that could be specified in the source code. We need to make a list of the specifics.

Add column-level filtering, where users can specify some text to filter one in a field just below the column header. If multiple columns have filter text, then the result is a intersection of all filtering criteria. This is something provided in the .net data grid.

I like the idea of a custom report template. That will be powerful capability.

Please note that there should be a default export for every enhanced browse. I would expect that the the user's choices about the look of the browse would be honored in any output. For example:

• font
• colors
• column sizing and order
• column hiding
• sorting and filtering

For PDF and Excel output, the default export should yield a formatted result that matches those choices. The customer should not have to create the report definition, I would expect us to create it automatically.

we may want consider a way of reporting out of the box> we may want consider a way of reporting out of the box

I was planning for a popup menu with an Export submenu that would have all 3 output options. The user would be prompted for the filename (client side) and any other needed output options (e.g. CSV separator).

Yes, we have context menu support already.

As noted above, I want the user to be able to override formatting choices like font, font color, cell background color, heading color, border color, whether a column is visible or not, the width of a column, the position (order) of the column.

We can save those changes on a per-user basis, so that for that browse on that screen, every time it will display the way the user has customized it.

We may even want some of those settings to be allowed to be globally set for the user.

I want the user to be able to override formatting choices like ... the width of a column

Do you mean column width relatively to other columns or ability to set actual Jasper print width in pixels?

The idea is that the user should be able to customize the look and feel of the actual browse columns in interactive mode. Resizing those to be smaller, larger. Hiding them if the user doesn't want to see the column.

Then when an export is done, these same choices should be honored by default. If a column is hidden it wouldn't be output to the report. The column's size would be honored in the report too.

Should in-application browse format (i.e. the current view) match output browse format or not?

Yes, that is the idea.

I assume "yes" means that formats match. So, when a column is hidden in report, it is hidden in the current browse too, right? And if we what a column back, we have a selection of columns that were defined for this browse (or added, if it is a dynamic browse), right?

So, when a column is hidden in report, it is hidden in the current browse too, right?

I would say it the other way. When the user hides a column in the current interactive browse AND then the user chooses to export, then the column would not be present in the report output.

And if we what a column back, we have a selection of columns that were defined for this browse (or added, if it is a dynamic browse), right?

Yes. The user can unhide any columns that have been hidden.

Consider user enters interactive mode. In that mode, browse is "replaced" with an interactive version of itself. Are non-exported columns hidden in this mode? Or they marked as "[non-exported]"? Am I right that the coloring, fonts and colors are the same for interactive and non-interactive modes? What about column headers: if original browse has no column headers, should we do something?

In that mode, browse is "replaced" with an interactive version of itself.

Do you mean replaced with an enhanced version of itself?

Are non-exported columns hidden in this mode?

The direction of changes goes from the interactive widget TO the exported report. It does not go the other direction.

At the moment that the user selects Export from the context menu, we would dynamically create a report definition that matches the current browse (including the formatting/visible columns/filtering/sorting...).

Or they marked as "[non-exported]"?

Anything non-hidden is exported. Anything hidden cannot be seen interactively and wasn't exported.

Am I right that the coloring, fonts and colors are the same for interactive and non-interactive modes?

Yes.

What about column headers: if original browse has no column headers, should we do something?

I didn't know that you could have a browse without column headers. How do you do that?

If the browse doesn't have headers, then the exported report would not have headers. The idea is that the user can customize the browse and that the default export will match the current interactive version of the browse. If the user hasn't customized it, then whatever was configured by the programmer would be output as the default. Either way, every enhanced browse in the system can be exported without any extra work by a user or by the programmer.

Do you mean replaced with an enhanced version of itself?

1. column resize is on;
2. column move is on;
3. you can right-click a column to set its font and coloring;
4. you can hide column;
5. you can show hidden columns.

I didn't know that you could have a browse without column headers. How do you do that?

NO-LABELS

Everything is correct except that I don't understand why the Age column exists in the final browse (bottom left to the right of "Finish Changes").

Do you mean replaced with an enhanced version of itself?

I mean a version where:

I think this is the same thing that I am calling "enhanced". It deviates from the 4GL compatible version and offers an updated, modern set of features. These are both visual features and functional features.

You can implement as a subclass of the BrowseGuiImpl or changes can be put directly in that class but activated only conditionally when the customer configures it to be active (via a flag in the directory).

It needs to operate without changes to the existing 4GL code. This means it needs to handle the setup and manipulation of the query and the event processing needs to work the same way. But the way the data is visualized, sorted, filtered would be different. Additional capabilities like page-level navigation and export would be provided.

The idea is that an entire application gets upgraded by setting a "activate-enhanced-browse" flag in the directory.

Are we talking about the same thing?

Are we talking about the same thing?

Yes.

Everything is correct except that I don't understand why the Age column exists in the final browse (bottom left to the right of "Finish Changes").

If we hide a column in interactive mode it is, obviously, hidden in the report. Is it hidden when we return to normal mode?

If we hide a column in interactive mode it is, obviously, hidden in the report. Is it hidden when we return to normal mode?

It is persistent in both interactive mode and exports, until the user choses otherwise. The formatting changes, hidden columns, sorting, filtering... all of the

All of these choices are not just for export.

Stanislav, Greg mentioned that you could use some help on "Automatically and sensibly expand to fit more space available when the frame and window are sized to allow it.". Can you provide some details?

This is the concept:

1. Detect if the containing window has empty space horizontally, vertically or both. If the browse is located at the top left of the window, then this is about the space to the right of and/or below the frame containing the browse. However, if the browse vertically or horizontally centered, then the space on all sides/top/bottom will matter.
2. Expand the size of both the containing frame and the enhanced browse to use that space.
3. Any other widgets to the side and top/below should shift positions based on the adjustment to the browse. The shift should be relative to the edge of the frame that is changing. The size of the other widgets would not change.
4. Let the enhanced browse use that extra space normally. It should show more rows and let the columns expand too.

Greg Shah wrote:

This is the concept:

On which events should the layout be changed? Is it after it is displayed, or its internal layout changed?

Greg Shah wrote:

This is the concept:

1. Expand the size of both the containing frame and the enhanced browse to use that space.

Do we want to support the more complex frame setups? Like enclosed frames or when there are sibling frames (whole frames shifted when space available)?

I should have mentioned that if the browse is not positioned at the top left and there is space in either of those dimensions, then the position and size of the browse will change. Otherwise, only the size will change, but not the position.

On which events should the layout be changed? Is it after it is displayed, or its internal layout changed?

I think the only times it needs to be changed:

• The first time it is positioned and sized.
• When the containing window is resized.

Do we want to support the more complex frame setups? Like enclosed frames or when there are sibling frames (whole frames shifted when space available)?

Yes. When this enhanced mode is active, all browses in the system should be handled.

If there are multiple browses in the window (side by side and/or above and below), they should share the space proportionally to their relative initial sizes.

This dynamic layout (if enabled) will be in effect only when a browse widget will appear in any of the affected frames? Does it hold true even for dynamic browse widgets?

This dynamic layout (if enabled) will be in effect only when a browse widget will appear in any of the affected frames?

Yes, for now.

Does it hold true even for dynamic browse widgets?

Yes.

Do we have a branch for this?

No, please create one.

Created task branch 3261a from P2J trunk revision 11200.

Rebased task branch 3261a from P2J trunk revision 11201.

We need to have the switch for enhanced browse features, so I created this container, searchable under Directory.ID_RELATIVE paths:

<node class="container" name="enhanced-browse">
   <node class="boolean" name="browse-reports">
      <node-attribute name="value" value="TRUE"/>
   </node>
   <node class="boolean" name="cell-hyperlinks">
      <node-attribute name="value" value="TRUE"/>
   </node>
   ...
</node> 

Stanislav Lomany wrote:

We need to have the switch for enhanced browse features, so I created this container, searchable under Directory.ID_RELATIVE paths:
[...]

I think that we only need to implement a single boolean flag that enables/disables the enhanced browse as a whole. I don't think that the individual features (e.g. reports or cell hyperlinking) need separate flags.

In regard to the cell hyperlinking, this one will need new 4GL syntax added. We could potentially support that "outside" of the enhanced browse mode if that syntax is used in the converted code. In other words, any use means that it is wanted by the programmer and can be implemented at runtime.

I was creating pop-up menu from a browse and met some issues:
1. As far as I understand file choose dialog (for choosing report output file) is in WIP state, so it works to a certain extent and in win8 theme only.
2. Sub-menus in popups have graphical bugs.
3. The main problem is that I met a bug with pop-up menus created on the client side (like in ScrollPopupGuiImpl.create): when a menu item is clicked, "Widget .. is not attached to a TopLevelWindow instance." may occur for a clicked item. I couldn't solve it quickly. Any advise on this? It doesn't block my work, but we need to do something with it on this week.

As far as I understand file choose dialog (for choosing report output file) is in WIP state, so it works to a certain extent and in win8 theme only.

There are improvements in branch 1830c. The cleanup work is not done, but it is close to completion.

As to the menu issues, we will put them on the list. Are they easy to reproduce? If you can open an issue for each one, I will make sure they get addressed.

Rebased task branch 3261a from P2J trunk revision 11214.

FYI. When printing from a browse, I have to create an intermediate result set containing all browse rows (all pages). I can try to get rows iteratively, but 1. it is not easy to refactor browse to support that. 2. before printed, jasper report is filled into JasperPrint object which contains all report rows, so we need an amount of memory anyway.

I think this is fine. It is good that you noted it.

Other than the memory usage, are there any compatibility issues created by the changes for the export?

No.

Good. Then we'll just list the added memory usage as part of the description of the export feature. When customers enable enhanced browse and use the feature, they will do it knowing this is a (temporary) consequence.

Very cool output!

Pagination. The options are: paginated on one sheet, paginated per sheets, no pagination.

I think there should be no pagination in this. The user will potentially be printing this and they can set their own pagination options. More importantly, the user will probably be working with the data. Any artificial pagination makes that harder.

White background behind report area.
Left/right 1-cell margin.
Report title.

For now, leave these as you have them. I will check with the customer. The extra left/right margin and the white background might not be needed. The report title does seem pretty important.

Make sure to also add CSV output.

The current work is in branch 3261a.

To enable "enhanced" browse with popups, set corresponding parameter in the directory or change the following code in BrowseWidget:

static
{
   Directory dir = DirectoryManager.getInstance();
   enhancedBrowse = dir.getBoolean(Directory.ID_RELATIVE,
                                   "enhanced-browse",
                                   false);
}

DEF TEMP-TABLE tt FIELD f1 AS INTEGER column-label "Label 1" 
                  FIELD f2 AS character format "x(20)" 
                  FIELD f3 AS LOGICAL
                  field f4 as character.

def var i as integer.
repeat i = 1 to 100:
   CREATE tt. tt.f1 = i.
   tt.f2 = "second " + string(i).
   tt.f3 = IF i MOD 2 = 1 THEN YES ELSE NO.
   tt.f4 = "text".
end.

DEFINE QUERY q FOR tt SCROLLING.
OPEN QUERY q FOR EACH tt.

DEF BROWSE br QUERY q
DISPLAY
     tt.f1
     tt.f2
     tt.f3
     tt.f4
   WITH size-chars 50 by 10 TITLE "Static browse" separators.

DEF FRAME fr br
WITH TITLE "Frame" SIZE 70 BY 15 NO-LABELS.

ENABLE ALL WITH FRAME fr.

WAIT-FOR WINDOW-CLOSE OF DEFAULT-WINDOW.

At this time the #3382 issue will be caused after using the export feature:

Caused by: java.lang.IllegalStateException: Widget -53 is not attached to a TopLevelWindow instance.
at com.goldencode.p2j.ui.client.widget.AbstractWidget.topLevelWindow(AbstractWidget.java:389)
at com.goldencode.p2j.ui.chui.ThinClient.invokeTriggers(ThinClient.java:18479)
at com.goldencode.p2j.ui.chui.ThinClient.invokeTriggers(ThinClient.java:18341)
at com.goldencode.p2j.ui.chui.ThinClient.processProgressEvent(ThinClient.java:17568)
at com.goldencode.p2j.ui.chui.ThinClient.processEventsWorker(ThinClient.java:16590)
at com.goldencode.p2j.ui.chui.ThinClient.pop(ThinClient.java:15595)
at com.goldencode.p2j.ui.chui.ThinClient.eventBracket(ThinClient.java:15578)
at com.goldencode.p2j.ui.chui.ThinClient.eventDrawingBracket(ThinClient.java:15502)
at com.goldencode.p2j.ui.chui.ThinClient.applyWorker(ThinClient.java:15263)
at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:12289)
at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:11774)
at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:11717)
at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:11671)
... 21 more

There is an issue with PDF output: when cell content is wider than the cell, it is improperly cut. Not sure if that can be fixed today.

Rebased task branch 3261a from P2J trunk revision 11291.

Greg, branch 3261a rev 11229 contains version with popup menu for reporting to a fixed file name.

3261a rev 11230 solves a build issue (jrxml were not included in p2j.jar)
3261a rev 11231 sends the report output (pdf, csv, etc) to the browser, to be downloaded.

Warning: first usage might require to 'allow popup windows from url...', as at least Chrome has popup-windows disabled.

Constantin Asofiei wrote:

3261a rev 11231 sends the report output (pdf, csv, etc) to the browser, to be downloaded.

For the demo, this change is fine. Eventually the need for an intermediate file should be eliminated by writing the report to a special-purpose stream. Similarly as GUIPrinterStreamSupport.Stream.

1. Word cutting in PDF - I made some investigations, and I'm not yet sure what is the correct solution.
2. Popup submenus are buggy: graphical artifacts and application freezes.
3. File selection for swing - buggy too, cannot enter target file name, missing images etc.
4. Font selection, color selection and file selection dialogs all have the "is not attached to a TopLevelWindow instance" issue.
5. OPEN-MIME-RESOURCE - if I understand correctly, there is no job on my part.
6. Implement storage in the directory where browse-specific report settings are saved.
7. Report title - most of the browses have no title, so I think we have to add ability to set the report title thru the popup menu.

File selection for swing - buggy too, cannot enter target file name, missing images etc.

If we are loading this directly to the browser/swing client, do we need to have a file name? The idea here is that we want to avoid that part since the common case is the web user that has no access to the client system.

If we are loading this directly to the browser/swing client, do we need to have a file name? The idea here is that we want to avoid that part since the common case is the web user that has no access to the client system.

For web we don't need a file name. But for swing I supposed we do. The only other option I see for swing is to save to the client's directory.

The same approach can be used in Swing to load the resource. For example, this works today with P2J-OPEN-URL. The idea is that OPEN-MIME-RESOURCE would do the same thing. It is using the browser to open the resource. The browser may still load a helper application (e.g. Libreoffice).

Greg Shah wrote:

The same approach can be used in Swing to load the resource. For example, this works today with P2J-OPEN-URL. The idea is that OPEN-MIME-RESOURCE would do the same thing. It is using the browser to open the resource. The browser may still load a helper application (e.g. Libreoffice).

Stanislav, you can leave the OPEN-MIME-RESOURCE on me. We already have a partial mechanism for delivering resources to clients used during printing. I will improve that to be generic for all (supported) MIME types and not just for printing, but browse, and possibly other places.

The idea is that OPEN-MIME-RESOURCE would do the same thing. It is using the browser to open the resource.

Great, less problems.

enhanced browse done by the end of March. This includes the other features like filtering, sorting and look and feel improvements

Filtering: is it a browse-level (we filter rendered results) or query-level filtering (we re-open the query)?
Sorting: this time we are alternating sort criteria for the query and re-open it, right?
Look and feel improvements: are the abilities on the diagram in the note 24 enough?

Filtering: is it a browse-level (we filter rendered results) or query-level filtering (we re-open the query)?

I was assuming it would be browse-level. We are just limiting what is being displayed to the user from the existing query results.

On any export, only the filtered results would be output.

Sorting: this time we are alternating sort criteria for the query and re-open it, right?

I'm not sure here. I was thinking it was something we would "overlay" on the results. The user would click on the column header to sort using that column. It would toggle between ascending and descending based on clicking a small triangle in the header. That triangle would point "up" when the sort was ascending and "down" for descending.

If you believe we need to modify the query and re-open it, then that is fine too.

Look and feel improvements: are the abilities on the diagram in the note 24 enough?

Yes. I am assuming that the user can resize both the column widths and the row height. Otherwise the features seem quite good.

Also, there is the hyperlinking feature from #3261-8.

There are two type of browse iterations: incremental (get next X rows) and "go to row Y". If we enable filtering, incremental scrolling is easy, but "go to row" is a problem because we have to filter all rows before the target one. It is easy to filter the rows once and then navigate the filtered result set (list of row ids in the original result set).
So, if we are ready to fetch and filter the full result set, the same approach can be used for sorting (without necessity of altering the BY clause).
Is it OK?
For filtering: are we going to have the additional "filtering" row of "cells" between column headers and first data row? Or we are going to extend the header height and include fill-in into it?

I have some concerns...

Greg Shah wrote:

Filtering: is it a browse-level (we filter rendered results) or query-level filtering (we re-open the query)?

I was assuming it would be browse-level. We are just limiting what is being displayed to the user from the existing query results.

I think this needs to be query-level, not browse-level. Since browse doesn't necessarily fetch the full result set at once, it would be very confusing to the user to filter down an existing subset of records (which is unknown to the user, as it is up to the implementation of the browse control) to a smaller subset of records.

For example, if a query has 50,000 results and the browse initially fetches 500, a filter might apply to 10% of the results. Unless we filter at the query level, we would be filtering down to 50, instead of the correct 5,000. Also, consider that this might be some seemingly random set of 50, depending on how the initial fetch was sorted.

Query-level filtering will be slower than just filtering what we already have fetched, but it's more correct.

So, in my mind the question is, how do we adjust an existing query (especially a complex join) to filter at the database?

I don't think it's a viable option to fetch all records and filter them in the application server. This can cause unacceptable performance issues for some queries. Remember how slow it was to filter at the application server before we had database-server-side UDF support? What if there are 500,000 results which meet the original query's criteria (or 5,000,000), combined with a column filter choice that is very restrictive?

On any export, only the filtered results would be output.

Sorting: this time we are alternating sort criteria for the query and re-open it, right?

I'm not sure here. I was thinking it was something we would "overlay" on the results. The user would click on the column header to sort using that column. It would toggle between ascending and descending based on clicking a small triangle in the header. That triangle would point "up" when the sort was ascending and "down" for descending.

If you believe we need to modify the query and re-open it, then that is fine too.

For the same reason as above (the browse's existing results may be a subset of the full query's results), I think it is necessary to modify the query and re-open it to make sure we are displaying the correct "bottom" or "top" of the full result set, not just re-sorting a subset and displaying a seemingly random collection of rows from the full result set.

But again, how do we apply new sort criteria to an existing query? We have a potential performance issue. Queries in applications generally have been written to leverage existing database indexes for performance. If we stick a new field at the beginning of a query's sort criteria, it is highly unlikely that it will leverage an existing index and could drastically change the query plan.

For example, if a query has 50,000 results and the browse initially fetches 500, a filter might apply to 10% of the results. Unless we filter at the query level, we would be filtering down to 50, instead of the correct 5,000. Also, consider that this might be some seemingly random set of 50, depending on how the initial fetch was sorted.

Through user interaction, the browse can cause any arbitrary number of the records (that can be returned by the attached query) to be displayed to the user. For example, although the initial fetch might be 500 rows, the user could page-down or scroll the browse in such a way as to force it to fetch all 50,000. This is built into the browse and not the application code itself.

This means that we can fetch the records as needed to obtain more for filtering, if the browse is intended to show a larger set of records and the filtering is coming up with a smaller set. Consider this a kind of simulated scrolling. It can stop soon as we get to the 500 filtered records or if we get to the end of the query.

For the same reason as above (the browse's existing results may be a subset of the full query's results), I think it is necessary to modify the query and re-open it to make sure we are displaying the correct "bottom" or "top" of the full result set, not just re-sorting a subset and displaying a seemingly random collection of rows from the full result set.

The sorting is more troublesome since it seems like all records must be fetched for this to ever work.

How often do browse queries actually pull back 50,000 records? Is this common?

I agree the result of filtering/sorting should be logically correct, consistent and intuitive to a user.

IMO query-based approach (clone the query with augmented WHERE or/and BY clauses) is more correct ideologically and memory efficient. But probably it is harder to implement and it will be less responsive than the browse-based approach.
Note there may be slight differences in how sorting and filtering work because in the case of browse-based implementation we may want to deal with the text representation of data rather than with the original values.
You decide.

BTW if content doesn't fit into a report cell, what should we do: extent cell height and break cell contents into multiple lines (breaking by word)?

There was error in reports when a text was not fitting a cell while it obviously should fit. Turned out that for some number output formats, generated text is left-padded with spaces to match the format length (e.g. "$ >>>9" is rendered into " $ 12" with two spaces in the beginning). That is correct when editing a value in a fill-in or a cell. But for BrowseRow.text which is used for reporting and rendering and searching non-editable cells I decided to left- or right-trim text depending on the alignment.

Stanislav Lomany wrote:

But for BrowseRow.text which is used for reporting and rendering and searching non-editable cells I decided to left- or right-trim text depending on the alignment.

This alignment will work only for fixed-width fonts. Isn't there a way to force alignment in the Jasper cell, and let the cell content text fully trimmed?

Isn't there a way to force alignment in the Jasper cell, and let the cell content text fully trimmed?

I'm not using spaces for alignment, Jasper has left- or right-aligned cells. The problem is that

[   $ 100]

[  .....$]
[     100]

Stanislav, have you tried disabling word wrap at cell level? I see some mentions here: https://github.com/handsontable/handsontable/issues/1460

Full trimming may reduce report size; also, if you export to CSV/Excel, won't the spaces interfere with how i.e. Excel/LibreOffice interprets numeric values?

Stanislav, have you tried disabling word wrap at cell level?

Disabling word wrap makes part of the text disappear. I'm waiting for Greg's answer on wrapping policy.

Full trimming may reduce report size;

I don't want full trimming because of the strings " which user for whatever reason may left-pad. He just likes padding."

also, if you export to CSV/Excel, won't the spaces interfere with how i.e. Excel/LibreOffice interprets numeric values?

I send into a report text rendered by browse. It is done to match the browse view.
Then Jasper tries to fit text in the cell cutting it if necessary. It affects all output formats in the same way.

Rebased task branch 3261a from P2J trunk revision 11221.

When browse enters "change layout" mode, how do we let user know that we are in this mode? We need to draw some label or, say, we can color browse scrollers.

1. Should they be stored on per-user basis?
2. Should the directory be used as the storage? Does it have enough throughput?
3. How are we going to uniquely identify browse in an app? Especially dynamic browse. My thought is "parent external procedure" + "set of fields which back the columns".
4. In layout editing mode we should have an option to undo changes. But there are two options to consider: 1. undo changes in the current editing session and 2. revert settings to default for this browse (set by 4GL code). Which of either options do we need?

query-based approach (clone the query with augmented WHERE or/and BY clauses) is more correct ideologically and memory efficient

Go with the query based approach for filtering and sorting. Make sure that the user has a way to reset the browse to the default (the original query that is unfiltered and has no user sorting).

That is correct when editing a value in a fill-in or a cell. But for BrowseRow.text which is used for reporting and rendering and searching non-editable cells I decided to left- or right-trim text depending on the alignment.

I agree that trimming here makes sense.

Stanislav, have you tried disabling word wrap at cell level?

Disabling word wrap makes part of the text disappear. I'm waiting for Greg's answer on wrapping policy.

I think how we handle word wrap may need to be specific to the output format. I guess I also need to understand how you are handling the sizing of the columns. There may be flexibility there too.

• CSV
  • I assume that we are using double quotes to enclose values so that the full range of characters can be safely included.
  • There is no concept of a column size in CSV so there should be no word wrapping here.
• XLS/XLSX
  • We do not need to match the column widths on a pixel level. The idea here is that this output is for editing and calculations, and not purely for printing. Even if it was for printing, the on-screen sizes of the browse (and its columns) are not things that we need to match.
  • The column sizes can be allowed to expand beyond the width of the screen (as shown in Excel or Libreoffice). The user can always use the spreadsheet's tools to scale and/or select printing ranges/options. In other words, We don't need to format the spreadsheet for printing.
  • For values up to a certain size, I would prefer to just let the column expand to be able to show everything. We need to figure out a good rule of thumb for that size limit.
  • For values over that size, we would want to allow the user to choose to enable word wrapping. In that mode we would change the height of those rows that must wrap (and only those rows).
• PDF
  • This one is primarily for printing purposes but the column sizes of the on-screen browse don't really matter. If needed to display something, the columns can be expanded to fit the width of the page (depending on portrait or landscape). We certainly should be taking up the full space available unless there is just a small set of columns. Perhaps the pixel width of the columns is used as a minimum value but not a maximum value.
  • If it can't be fit on a single page width, I guess we let it flow over to other pages. How do we handle this today?
  • For values up to a certain size, I would prefer to just let the column expand to be able to show everything. We need to figure out a good rule of thumb for that size limit.
  • For values over that size, we would want to allow the user to choose to enable word wrapping. In that mode we would change the height of those rows that must wrap (and only those rows).

We could also enable word wrap by default for text over a certain size and then allow the user to disable it. In any scenario where word wrap is disabled, we should display as much as possible and then truncate the rest.

We might also need to allow the user to control the size limit to which we will expand any columns.

When browse enters "change layout" mode, how do we let user know that we are in this mode? We need to draw some label or, say, we can color browse scrollers.

I'm open to different ideas. A special outline box could be drawn around the browse. A tooltip could be displayed to notify the user and explain how to exit that mode.

I would expect the browse content to be fixed/read-only at that point.

Should they be stored on per-user basis?

Yes.

Should the directory be used as the storage?

Yes.

Does it have enough throughput?

I think so, especially if we provide a higher leverage approach. When the user makes changes and decides to save them, perhaps we can give them the option to make those settings the default for all browses. This way, the most common changes can be "global" for the user.

Then the number of places that need to be changed to customize specific browse instances can be much reduced. In this scenario, the average user will only customize a small number of browses that they use often. It should be manageable.

My thought is "parent external procedure" + "set of fields which back the columns".

I like it.

1. undo changes in the current editing session and 2. revert settings to default for this browse (set by 4GL code). Which of either options do we need?

Both seem pretty useful. The first is just a "cancel" on the change layout mode. The second clears all previously applied changes for this specific browse instance. I think they are both needed.

Greg Shah wrote:

query-based approach (clone the query with augmented WHERE or/and BY clauses) is more correct ideologically and memory efficient

Go with the query based approach for filtering and sorting. Make sure that the user has a way to reset the browse to the default (the original query that is unfiltered and has no user sorting).

It probably makes sense to add an API (or two) to P2JQuery to allow each query type to perform this augmentation in the most appropriate way for its implementation.

It probably makes sense to add an API (or two) to P2JQuery to allow each query type to perform this augmentation in the most appropriate way for its implementation.

This is a great idea.

What are the next set of changes that will be available in 3261a? When will they be ready for review?

I would like to get the changes in this branch into the trunk at that time.

What are the next set of changes that will be available in 3261a? When will they be ready for review?

I think the right-click "enhanced" menu will be fully available around the middle of the next week.
So far I had no time to completely fix the report word wrap issue.

1. Should user be able to choose an arbitrary color or a color only from the color table?
2. At first I didn't notice that font selection dialog is a way to update a font from the font table rather than select a font from the font table. Do we have something that can help to choose a font?

Should user be able to choose an arbitrary color or a color only from the color table?

Any color. The Progress color table is limiting and makes no sense. Use the color chooser dialog. Please note that there are pending fixes in 1830c.

At first I didn't notice that font selection dialog is a way to update a font from the font table rather than select a font from the font table. Do we have something that can help to choose a font?

We have our own replacement for the font dialog. The 1830c is the branch to use.

Ovidiu: I assume we update the font table the same way that the 4GL does?

Greg Shah wrote:

Should user be able to choose an arbitrary color or a color only from the color table?

Any color. The Progress color table is limiting and makes no sense. Use the color chooser dialog. Please note that there are pending fixes in 1830c.

It is a bit strange, but I don't think that the current implementation of SYSTEM-DIALOG COLOR is able to provide you directly with the color (as RGB stored in an integer) you desire. The ColorChooserDialog.execute() lets the client display the dialog and waits for the result. The selected color will be stored in the palette at the requested index. For an ABL usage example, please see the ABL Reference, SYSTEM-DIALOG COLOR statement.

At first I didn't notice that font selection dialog is a way to update a font from the font table rather than select a font from the font table. Do we have something that can help to choose a font?

We have our own replacement for the font dialog. The 1830c is the branch to use.

1830c only improves usability of the dialogs already existing in trunk. For an ABL usage example, please see the ABL Reference, SYSTEM-DIALOG FONT statement. If you want to integrate it in new code, please check #3290-1 to see how the code is converted.

Ovidiu: I assume we update the font table the same way that the 4GL does?

I guess so. The color and fonts are stored in their tables at the specified location (indexes). You need to set them before creating the widgets or otherwise update the UI because the new values are not automatically loaded by widgets. Please let me know if you have any questions.

I don't think that the current implementation of SYSTEM-DIALOG COLOR is able to provide you directly with the color (as RGB stored in an integer) you desire

Good point.

I think this is a place where we can extend the dialog to add an RGB mode which just returns the RGB value without making any changes to the color table.

The color and fonts are stored in their tables at the specified location (indexes). You need to set them before creating the widgets or otherwise update the UI because the new values are not automatically loaded by widgets.

Why not extend this to add a "direct" mode that just returns back the chosen font name instead of making the font table change.

With these features, the enhanced browse can call them and get back the values needed without having to use platform-specific dialogs.

Greg Shah wrote:

I think this is a place where we can extend the dialog to add an RGB mode which just returns the RGB value without making any changes to the color table.

The color and fonts are stored in their tables at the specified location (indexes). You need to set them before creating the widgets or otherwise update the UI because the new values are not automatically loaded by widgets.

Why not extend this to add a "direct" mode that just returns back the chosen font name instead of making the font table change.

With these features, the enhanced browse can call them and get back the values needed without having to use platform-specific dialogs.

OK, I will add a worker method that gets the RGB from client side. If the RGB is needed it is accessed directly, and the execute method will just store it in the COLOR-TABLE.

1. Modify browse to add additional RGB color properly for each colored types of elements.
2. Modify browse to add additional font properly for each type of text elements.
3. TC.chooseFont() version with returns FontDetails.

Right?

Stanislav Lomany wrote:

Guys, TC.chooseColor() already returns RGB color as I need.

Indeed, TC.chooseColor() returns the selected RGB or -1 if user rejected the dialog.

Am I correct that the chosen font in this case is not bound to the fonts table and is directly applied to the browse element? In this case we need to

1. Modify browse to add additional RGB color properly for each colored types of elements.
2. Modify browse to add additional font properly for each type of text elements.
3. TC.chooseFont() version with returns FontDetails.

Right?

Please see TC.chooseFont(). The result (found in fcgi[0].getSelectedFont()) will overwrite the current font in FontManager at entry fontNum. The server side is just notified whether the update was successful or not. Do you need the FontDetails on server side?

If my assumption is correct (Greg?), I need version with takes FontDetails as input, doesn't change the font table, and returns new FontDetails. I don't need them on server.

The colors and fonts chosen by the user are overrides that are directly applied to the associated browse feature being customized by the user. We do not want any changes to the color table or font table.

Am I correct that the chosen font in this case is not bound to the fonts table and is directly applied to the browse element?

Yes.

Rebased task branch 3261a from P2J trunk revision 11229.

Greg, please review 3261a revision 11229.

Code Review Task Branch 3261a Revision 11245

This is a good result. I find it especially interesting that you leveraged XmlAst instead of direct DOM usage. I guess it is simpler to use. I also like that the code has better factoring.

I did check in some minor formatting and text changes as revision 11246.

1. Can this solution handle when there is a browse column that is calculated (not directly mapped to a field from a query)? I don't see anything obvious in the solution to handle that case.

2. When we implement ProDataSet support, there may be some confusion between a "report data-source" and a "ProDataSet data-source". For now, I don't think we need to worry about this unless you have an obvious idea of how to change the terminology.

3. In BrowseColumnWidget.setVisible(), is there a need to have null pointer check on the return value from getBrowse()?

4. BrowseJasperDataSource.next() and BrowseJasperDataSource.getFieldValue() may need null pointer protection before dereferencing browseRows.

5. The throws JRException should be split onto a separate line in BrowseJasperDataSource. This effects methods next() and getFieldValue().

1. Can this solution handle when there is a browse column that is calculated (not directly mapped to a field from a query)? I don't see anything obvious in the solution to handle that case.

Yes: report takes data from browse after triggers (and calc columns) have been processed.

3. In BrowseColumnWidget.setVisible(), is there a need to have null pointer check on the return value from getBrowse()?

4. BrowseJasperDataSource.next() and BrowseJasperDataSource.getFieldValue() may need null pointer protection before dereferencing browseRows.

5. The throws JRException should be split onto a separate line in BrowseJasperDataSource. This effects methods next() and getFieldValue().

Fixed it.

I don't think the changes are checked in.

Now they are.

The changes are good.

Please get this tested.

For 3261b, please make a list of the next set of changes to be implemented. I prefer to deliver something by the end of next week.

1. Finish with font/color selection.
2. I think I'll do indication of layout mode with thick red dotted line on the browse edges (within browse drawing area).
3. Saving of browse settings.
4. Undo of settings.

Are these included in next week's deliverable?

• filtering/sorting
• hyperlinking (#3261-8)

I'll only start them. I can start with hyperlinks - that gives us a chance to get it done by the end of the next week.

Rebased task branch 3261a from P2J trunk revision 11230.

Regression testing passed for version based on rev 11229.
Made some visual testing with customer app.
Should I check into trunk?

Please merge 3261a to trunk.

3261a has been merged into the trunk as bzr revision 11231.

Created task branch 3261b from P2J trunk revision 11231.

Rebased task branch 3261b from P2J trunk revision 11232.

3261b has been merged into the trunk as bzr revision 11238.

Created task branch 3261c from P2J trunk revision 11238.

1. add column option HYPERLINK "event-name";
2. make text inside all cells of the hyperlinked columns blue (?) and underlined (?);
3. text hovering cursor should be "hand";
4. on clicking a cell's text, PUBLISH "event-name" "cell-text" is executed;
5. for editable browse you can click on the text padding area within the cell to start editing (instead of firing PUBLISH).

About enhanced browse font adjustment: should column width be adjusted accordingly when we increase or decrease font size? What if the column has an explicit width specified?

Stanislav Lomany wrote:

These are my thoughts, but I'd like Greg's input before you implement...

About hyperlinking: do we want to implement the following?
add column option HYPERLINK "event-name";

Yes.

make text inside all cells of the hyperlinked columns blue (?) and underlined (?);

I don't know if the idea is to have the text always underlined and colored, or only on hover. If the latter, it would make the distinction more clear for editable cells between when a click means hyperlink and when it means edit, though the change in mouse pointer may be indication enough. I'm a bit concerned it may become a source of UX frustration to maneuver the mouse to exactly the correct location to discern between one action and another.

Anyway, w.r.t. the original question, I think yes, whether it is on hover or all the time, the link is denoted by some shade of blue (theme-specific) and an underline.

text hovering cursor should be "hand";

Yes; the "link select" hand.

on clicking a cell's text, PUBLISH "event-name" "cell-text" is executed;

Yes, I believe this is the idea.

for editable browse you can click on the text padding area within the cell to start editing (instead of firing PUBLISH).

See comments above.

About enhanced browse font adjustment: should column width be adjusted accordingly when we increase or decrease font size? What if the column has an explicit width specified?

Also, we may want to change row height accordingly.

Stanislav Lomany wrote:

About enhanced browse font adjustment: should column width be adjusted accordingly when we increase or decrease font size?

Also, we may want to change row height accordingly.

Yes, I think we have to change both column width and row height if the font size changes. Is this specific to the enhanced version?

What if the column has an explicit width specified?

In what case isn't the width explicit? Isn't it always the case? Or, to ask it differently, isn't the width always implicitly a factor of the font size?

Yes, I think we have to change both column width and row height if the font size changes. Is this specific to the enhanced version?

Yes, specific only to "change font" option of the enhanced version.

In what case isn't the width explicit? Isn't it always the case? Or, to ask it differently, isn't the width always implicitly a factor of the font size?

Width can be implicit (factor of the font size) or explicit (font-independent):

DISPLAY 
     tt.f1 
     tt.f2 width 10
     tt.f3 width-pixels 40

make text inside all cells of the hyperlinked columns blue (?) and underlined (?);

I don't know if the idea is to have the text always underlined and colored, or only on hover. If the latter, it would make the distinction more clear for editable cells between when a click means hyperlink and when it means edit, though the change in mouse pointer may be indication enough. I'm a bit concerned it may become a source of UX frustration to maneuver the mouse to exactly the correct location to discern between one action and another.

I don't think we care to only do this on hover. Make it blue and underlined like a browser link.

I'm good with the rest of the plan as discussed above.

Width can be implicit (factor of the font size) or explicit (font-independent):

When the width is implicit, changing font size should recalculate the column width.

When the width is explicit, we should leave it set to that same value unless the user has manually resized the columns.

• Enhanced state is applied.
• Browse is displayed.
• Column set is adjusted by 4GL code.
• User is editing browse to create the enhanced state from the item 1.

I think we can identify columns by the referenced buffer field. For calc columns we can use parameters used at creation time.

I think we can identify columns by the referenced buffer field. For calc columns we can use parameters used at creation time.

OK.

Guys, who is proficient in fonts? How can I display a text in FWD using an arbitrary font which doesn't exist in a font table (I have FontDetails)?

Stanislav Lomany wrote:

Guys, who is proficient in fonts? How can I display a text in FWD using an arbitrary font which doesn't exist in a font table (I have FontDetails)?

If you have the FontDetails, than just make sure to call GuiDriver.setGuiFont - this will be the font used for all next UI operations.

Sergey: Can you help Stanislav with his request in #3261-124?

Greg Shah wrote:

Sergey: Can you help Stanislav with his request in #3261-124?

Yes, I can take it. What is the priority of this task? Now I am working on 3551 to prepare a patch for ScopDir.i and then to test the converted code deeply.

Sergey Ivanovskiy wrote:

Greg Shah wrote:

Sergey: Can you help Stanislav with his request in #3261-124?

Yes, I can take it. What is the priority of this task? Now I am working on 3551 to prepare a patch for ScopDir.i and then to test the converted code deeply.

Please work out the ScopDir.i patches first. Then you should have a natural break to work on the #3261-124 issues while the code is converting.

Stanislav Lomany wrote:

...and row resizing has a complex of issues

Stas, can you be more specific as to what needs to be fixed?

OK. I had it in mind.

Stanislav,
For latest issue, I can only confirm that the Swing client is handling fine the resize and column reorder events.

1. create a set of couple of methods in BrowseWidget for adding a filter and a sort order for a specified column. This will be somehow called when the user clicks on the filtering/sorting header of respective column;
2. create the complementary methods of clear all existing filter and a sort order for a browse;
3. as part of the implementation of these methods, I need to use the provided column number to identify the actual field and table that user is interested in. This is a bit tricky at this moment as the BrowseColumnWidget does not have much information about the field it displays (only a DMO interface is available and data type);
4. for sorting, I think prepending the field to the by clause will do the trick;
5. to enable filtering, the best solution would be to identify the query component of the respective buffer and create a new where predicate where the old one is AND- ed with a check for requested value. I guess this can be a BDT which will probably inserted in the array of parameters for that HQL.

It is not very clear for me whether we can use the existing query or we need to derivate new queries based on the user requests. If we create other temporary queries, the external changes to query will not be reflected in the browse content. If we alter the current query it might be difficult to revert back changes on clear filter/sort.

Ovidiu Maxiniuc wrote:

create a set of couple of methods in BrowseWidget for adding a filter and a sort order for a specified column. This will be somehow called when the user clicks on the filtering/sorting header of respective column;

No, this is Stanislav's part.

create the complementary methods of clear all existing filter and a sort order for a browse;

No, Stanislav's part.

as part of the implementation of these methods, I need to use the provided column number to identify the actual field and table that user is interested in. This is a bit tricky at this moment as the BrowseColumnWidget does not have much information about the field it displays (only a DMO interface is available and data type);

No, Stanislav's part.

for sorting, I think prepending the field to the by clause will do the trick;

Yes, this is the current thinking, though I'm a little concerned about performance. Regardless of that concern, I think this is the only way to do it.

to enable filtering, the best solution would be to identify the query component of the respective buffer and create a new where predicate where the old one is AND- ed with a check for requested value. I guess this can be a BDT which will probably inserted in the array of parameters for that HQL.

This is the current thinking, but it may get complicated with multi-table queries.

It is not very clear for me whether we can use the existing query or we need to derivate new queries based on the user requests. If we create other temporary queries, the external changes to query will not be reflected in the browse content. If we alter the current query it might be difficult to revert back changes on clear filter/sort.

I don't know the right answer for this, but it will be driven by the BrowseWidget. Again, Stanislav's part, but we should collaborate to figure out the best solution. We probably can do something with the QueryWrapper.delegate here.

Stanislav: currently, BrowseWidget references P2JQuery, but isn't it actually limited to QueryWrapper instances? If we can limit it to using QueryWrapper instead of P2JQuery, this will give us some options in letting QueryWrapper manage the delegate.

Let's get the QueryWrapper question above resolved first. Assuming we can swap in QueryWrapper for P2JQuery in BrowseWidget, your task is to create an API at the QueryWrapper level to enable filtering and sorting, and to implement that API at the various concrete P2JQuery subclasses whose instances can be QueryWrapper delegates.

Stanislav, what parameters do propose get passed to the filter API? A FieldReference and a match value; or an HQL WHERE clause expression snippet; or something else? Same question for the sort API.

Stanislav Lomany wrote:

Stas, can you be more specific as to what needs to be fixed?

On the screenshot I was doing row resize. I've positioned the cursor over the row separator and dragged it down and a bit to the right.

1. Dragging is stopped at some point, you can see the difference between bottom of the rectangle representing the resized row and the pointer.
2. When I released mouse button, nothing happened: selection remained, the row hasn't been changed.
3. It cannot be seen on the screenshot, but the cursor during resize is incorrect: it should be "resize arrow pointing down", while it was "hand".

I tested this issue and found that the most right side of the resize box can't be dragged and if the cursor is over this side, then it doesn't change its shape to the corresponding resize cursor shape. But its bottom side can be dragged and I didn't find another issues except this one.
It seems that the any side of the resize box except a bottom side can't be dragged in the web client. Thus bottom sides of cells can be dragged only. The Swing client makes possible to drag any cell side. Planning to investigate this issue.

I just found that for the web client right sides of cells also can be dragged but the mouse pointer is not changed if the mouse is over this bound.

It seems that a widget should implement at least these methods in order its mouse pointer shape will react on mouse hover events

   /**
    * Check if the widget supports LOAD-MOUSE-POINTER() method 
    * 
    * @return {@code true} if the widget supports LOAD-MOUSE-POINTER() method
    */
   public boolean supportsCustomMousePointer();

   /**
    * Create handler for processing mouse hover events
    * 
    * @return handler for processing mouse hover events
    */
   protected MouseHoverAction createMouseHoverAction();

3261c
src
com.goldencode.p2j.ui.client.gui
EditorGuiImpl
supportsCustomMousePointer()
ScrollPaneGuiImpl
supportsCustomMousePointer()
com.goldencode.p2j.ui.client.gui.driver.web
GuiWebDriver
getWebActions(Widget)
com.goldencode.p2j.ui.client.widget
AbstractWidget<O extends OutputManager<?>>
_setEnabled(boolean)

In 3261c the mouse pointer changes are controlled by BrowseColumnGuiImpl and the mouse shape is changed on mouse events by this method

 
   /**
    * Notification of a mouse moved event occurred for this widget.
    *
    * @param    e
    *           The mouse event.
    */
   @Override
   public void mouseMoved(MouseEvent e)
   {
      // switch to resize cursor if required
      if ((browse.isColumnResizable() || config().resizable) && !markersColumn)
      {
         NativePoint p = translate(e);

         if (browse.getRowResizeArea(p) != null)
         {
            // row resize is handled by MouseDirectManipulation
            currentResizeArea = null;
            return;
         }

         if (getVisibleColumnIndex() >= browse.config().numLockedColumns &&
             browse.isLastLockedColumnResizeArea(e))
         {
            gd.setCursor(MousePtrWrapper.E_RESIZE);
            currentResizeArea = ResizeArea.LAST_LOCKED_RIGHT;
         }
         else if (hasLeftResizeArea() && p.x >= 0 && p.x < COLUMN_RESIZE_AREA_WIDTH)
         {
            gd.setCursor(MousePtrWrapper.E_RESIZE);
            currentResizeArea = ResizeArea.LEFT;
         }
         else if (hasRightResizeArea() &&
                  p.x >= physicalDimension().width - COLUMN_RESIZE_AREA_WIDTH &&
                  p.x < physicalDimension().width)
         {
            gd.setCursor(MousePtrWrapper.E_RESIZE);
            currentResizeArea = ResizeArea.RIGHT;
         }
         else
         {
            gd.setCursor(MousePtrWrapper.DEFAULT);
            currentResizeArea = null;
         }
      }
   }

Also I found that MouseDirectManipulation java logic doesn't change the cursor shape.

If we drag rapidly the bottom side of the resize box until it passes more than 3 rows, then it throws this error. It can be reproduced sometimes.

This error is an 4GL error and appears if the new row height is too large (see error text for details).

Stanislav Lomany wrote:

If we drag rapidly the bottom side of the resize box until it passes more than 3 rows, then it throws this error. It can be reproduced sometimes.

This error is an 4GL error and appears if the new row height is too large (see error text for details).

Do you mean that it is a correct 4GL behaviour?

Yes.

currently, BrowseWidget references P2JQuery, but isn't it actually limited to QueryWrapper instances?

Yes, I think it is limited to QueryWrappers.

Stanislav, what parameters do propose get passed to the filter API? A FieldReference and a match value; or an HQL WHERE clause expression snippet; or something else? Same question for the sort API.

For filtering:
Array of handles to buffer fields (or FieldReferences) and array of filter values (preferably as strings).

For sorting:
Array of handles to buffer fields (or FieldReferences) and asc/desc sorting as array of booleans. Earlier position in array means more coarse sorting.

Stanislav Lomany wrote:

currently, BrowseWidget references P2JQuery, but isn't it actually limited to QueryWrapper instances?

Yes, I think it is limited to QueryWrappers.

Does anyone have a concern about encapsulating the "query switching" needed for filter/sort operations within QueryWrapper?

If not, Stanislav, please replace references to P2JQuery in BrowseWidget with QueryWrapper and confirm it still works. I expect it should, since QueryWrapper implements P2JQuery, and I can't think of any code (hand-written or converted) that passes a P2JQuery instance to it. Can you?

Eric Faulhaber wrote:

Does anyone have a concern about encapsulating the "query switching" needed for filter/sort operations within QueryWrapper?

Not for the moment, but I am working on this. I'll post any issue that blocks me.

If not, Stanislav, please replace references to P2JQuery in BrowseWidget with QueryWrapper and confirm it still works. I expect it should, since QueryWrapper implements P2JQuery, and I can't think of any code (hand-written or converted) that passes a P2JQuery instance to it. Can you?

I already did that.. seems to work.

• what is the best API: pass to QueryWrapper one filter at a time and keep the management in BrowseWidget or the BrowseWidget independently add one filter at a time and management is kept in QueryWrapper? I incline to latter, to keep BrowseWidget cleaner;
• I think it would be better for filtering to pass in the actual BDT, not its string representation. If it is really not available, I can rebuilt the BDT using the string c'tor. However, the value might not be correctly re-hydrated;
• what happens with computed columns? Should we filter / sort based on them? For example:
  

  DEF BROWSE br QUERY q 
  DISPLAY 
       tt.f1     COLUMN-LABEL "field-ref!column" 
       2 * tt.f1 COLUMN-LABEL "computed!column" 
     WITH size-chars 70 by 10 TITLE "Static browse" SEPARATORS.
  

  
  As a user, I would expect to apply sort/filter on all columns of the table. For the moment, BrowseWidget is not aware of the formula used for a specified column. But, even if it was, the fields might be form different databases... This is getting ugly.

• what happens with computed columns? Should we filter / sort based on them? For example:

It is impossible to filter or sort by computed columns. Their values are set in ROW-DISPLAY trigger, so we don't know values for rows outside the view.

Sorry, I missed that these are computed columns. I was referencing calc columns.

Do we need to implement lambdas for the expression in a computed column? This way the browse can calculate it whenever it needs to display the row.

How would we use lambdas to augment an order by (sorting) or where clause (filtering), since we need to do these on the database server?

Eric Faulhaber wrote:

How would we use lambdas to augment an order by (sorting) or where clause (filtering), since we need to do these on the database server?

Database server processing disallows sorting/filtering on computed columns. But the idea is that the browse can maintain correct computed column output when the filtering/sorting changes.

Or we can use client-side where clauses/sorting which would allow full usage of the computed column lambda.

Stanislav: I guess we may already pass an anonymous inner class instance of these expressions so that the browse can calculate the column values as needed? If so, then the lambda is not really needed.

Eric Faulhaber wrote:

How would we use lambdas to augment an order by (sorting) or where clause (filtering), since we need to do these on the database server?

Ultimately, we could use a HQL-like string generated at conversion time and somehow passed together with the lambda.

LE: this HQL (and selected value for filtering) will be injected in the query (before re-preoprocessing). The browse won't do any additional computations, just to pass the HQLs, in case of a computed column.

• I think it would be better for filtering to pass in the actual BDT, not its string representation. If it is really not available, I can rebuilt the BDT using the string c'tor. However, the value might not be correctly re-hydrated;

Right, let's go with BDT.

Stanislav: I guess we may already pass an anonymous inner class instance of these expressions so that the browse can calculate the column values as needed? If so, then the lambda is not really needed.

We have lambdas as computed expressions:

new HeaderElement(() -> multiply(2, tt.getF1()), frFrame.widgetBrExpr3Column())

Note that in order to compute a computed cell value, we have to be positioned on the required row. So filtering/sorting by computed columns is full fetch of the data set.

Computed columns can be not only formulas, but user-written functions, which can run sub-queries etc. So caching computed data or altering HQL somehow is not an option.

Stanislav Lomany wrote:

Computed columns can be not only formulas, but user-written functions, which can run sub-queries etc. So caching computed data or altering HQL somehow is not an option.

Indeed, the UDFs are bad in this context, making sorting/filtering near to impossible. The solution would be to bring the whole result set and processing it on the application server and do a client-side processing, which is as bad as possible.

Ovidiu Maxiniuc wrote:

Stanislav Lomany wrote:

Computed columns can be not only formulas, but user-written functions, which can run sub-queries etc. So caching computed data or altering HQL somehow is not an option.

Indeed, the UDFs are bad in this context, making sorting/filtering near to impossible. The solution would be to bring the whole result set and processing it on the application server and do a client-side processing, which is as bad as possible.

An idea: apply this 'client-side' filtering (UDFs and other stuff, for computed columns) on the databse-server filtered result set. And if the result set is 0 or smaller than a certain threshold, bring another 'page'. As I recall, the browse already has a 'scrolling' behaviour, where only a certain number of rows is brought implicitly and more rows are pulled when the browse is scrolled.

As a reminder about performance: #3261#note-67

If we want to support computed columns, then we have to go with the scrolling approach for filtering as Constantin just suggested and full fetch for sorting.
If we have a computed column, we can handle other columns in the same way, not much sense to alter HQL in this case.
If we have no computed columns in the browse, we can go with HQL approach.

We will have two methods to perform filtering/sorting: fast and computed-slow.
What do we want to implement first?

Constantin Asofiei wrote:

An idea: apply this 'client-side' filtering (UDFs and other stuff, for computed columns) on the databse-server filtered result set. And if the result set is 0 or smaller than a certain threshold, bring another 'page'. As I recall, the browse already has a 'scrolling' behaviour, where only a certain number of rows is brought implicitly and more rows are pulled when the browse is scrolled.

Correct! This works for filtering. We don't even need to keep the expression evaluator or even to alter the queries, just need to drop records that don't match the filter value. Ideally this should be done only when the expression is to complicated to be coded as a HQL/SQL, in order to fetch as fewer rows as possible to application server.

The sorting, however, is impossible this way, the (vertical) location of each row depends on the entire content of the result set.

Sergey Ivanovskiy wrote:

It seems that a widget should implement at least these methods in order its mouse pointer shape will react on mouse hover events
[...]
I did a search for supportsCustomMousePointer() and found these references
[...]
Thus a table cell should be a Widget, then the java web client registers and updates its widget's mouse region in the JS web client. And the JS web client having this region can update the mouse pointer shape when the mouse is over this region.

In 3261c the mouse pointer changes are controlled by BrowseColumnGuiImpl and the mouse shape is changed on mouse events by this method
[...]
It seems that we can't change the cursor shape for the web client without special efforts. The JS web client should be aware about cell regions or at least about the current resizable region. For an example, the current resizable region can be registered and updated by the web client.

At the moment I have no a concrete plan how to implement this absent functionality. Planning to notify as soon as possible when it will be ready.

Sergey Ivanovskiy wrote:

Also I found that MouseDirectManipulation java logic doesn't change the cursor shape.

I did't know MouseDirectManipulation java logic and missed that this class can change mouse pointer. For the web this flow could be observed

MouseDirectManipulation.mouseMoved(MouseEvent) line: 488    
MouseHandler.processWidgetActions(int, MouseEvent) line: 456    
MouseHandler.handleMouseEvent(int, MouseEvent) line: 295    
GuiWebDriver(AbstractGuiDriver<F>).handleMouseEvent(int, MouseEvent) line: 3023    
WindowManager.processWindowEvent(Event, TopLevelWindow<?>) line: 1716    
WindowGuiImpl.processEvent(Event) line: 1366    
ThinClient.processProgressEvent(Event) line: 17326    
ThinClient.processEventsWorker() line: 16750    

   public void mouseMoved(MouseEvent e)
   {
      // if we are in any of the dragged mode suppress pointer change
      if (widgetResizing || rowResizing || widgetsMoving || boxSelecting)
      {
         return;
      }

      MousePtrWrapper ptrNew = null;
      // the pointer can be changed either by widget resize handles or by browse row resize
      if (isFrame || isWindow)
      {
         ptrNew = getNewPointer(e);
      }
      else if (isInsideRowResizable())
      {
         ptrNew = getRowResizePointer(e);
      }
...........................................

  /**
    * Checks if new mouse pointer for browse row resize is need to be used.
    * 
    * @param    e
    *           The mouse event to check.
    * 
    * @return   New mouse pointer or default one from the set predefined in
    *           {@link MousePtrWrapper}.
    */
   private MousePtrWrapper getRowResizePointer(MouseEvent e)
   {
      return isInsideRowResizeArea(e) ? MousePtrWrapper.S_RESIZE : MousePtrWrapper.DEFAULT;
   }

   /**
    * Checks if inside the row resizable widget.  The browse column is an example. 
    * 
    * @return   TRUE if the point is inside row resizable widget,
    *           FALSE otherwise.
    */
   private boolean isInsideRowResizable()
   {
      return widgetEff instanceof BrowseColumnGuiImpl &&
             ((BrowseColumnGuiImpl) widgetEff).getBrowse().isRowResizable();
   }

Thus we can see that mouse pointer can be default or S_RESIZE. It is not clear for me why another sides of "resize" area are not handled by this code.

IIRC, browse columns resize functionality was implemented by me before we had MouseDirectManipulation class, and row resize was implemented be Eugenie when he created MouseDirectManipulation.

1. DirectoryService ds = DirectoryService.getInstance(); gets you access to the directory - use APIs to either get or set node attributes. Note that get/set works with existing nodes; when the node doesn't exist, you need to add it (i.e. ds.addNodeString). Greg, do we need something specific to flush the directory changes to disk? Or ds.unbind() will take care of saving these? More, what about concurrent acces - does this work by default, even if multiple users edit the same node?
2. Utils.findDirectoryNodePath - use this to identify a certain directory node, on a per-user or per-server basis. If you set the scope argument to true, it will search under i.e. /server/{default|<server-ID>}/runtime/<account-id>/enhanced-browse-config, so you can make it user-specific.
3. I advise adding a dir_schema.xml node class configuration, so it will be easier to define these in the directory; for appserver, I have something like:
   

             <node class="appserver" name="app_server">
               <node-attribute name="operating_mode" value="state-reset"/>
               <node-attribute name="request_timeout" value="15"/>
               <node-attribute name="auto_trim_timeout" value="30"/>
               <node-attribute name="propath" value=".:"/>
               <node-attribute name="initial_agents" value="10"/>
               <node-attribute name="min_agents" value="10"/>
               <node-attribute name="max_agents" value="10"/>
               <node-attribute name="activate" value=""/>
               <node-attribute name="deactivate" value=""/>
               <node-attribute name="connect" value=""/>
               <node-attribute name="disconnect" value=""/>
               <node-attribute name="startup" value=""/>
               <node-attribute name="shutdown" value=""/>
               <node-attribute name="startup_parameter" value=""/>
             </node>
   

4. for colors, why not use the CSS hex format? Is easier to understand/use.

Greg, do we need something specific to flush the directory changes to disk? Or ds.unbind() will take care of saving these?

When editing the directory, you must bracket the edits in between DirectoryService.openBatch() and DirectoryService.closeBatch() calls. That is what handles persistence.

You can also check DirectoryService.isEditing().

More, what about concurrent acces - does this work by default, even if multiple users edit the same node?

Editing is serialized. While one session is editing, other sessions are locked out.

Greg Shah wrote:

Editing is serialized. While one session is editing, other sessions are locked out.

I see, synchronized(this) in DirectoryService.openBatch() solves the concurrency issue. And DirectoryService has a single instance per server, so we are good here.

Stanislav Lomany wrote:

Thus we can see that mouse pointer can be default or S_RESIZE. It is not clear for me why another sides of "resize" area are not handled by this code.

IIRC, browse columns resize functionality was implemented by me before we had MouseDirectManipulation class, and row resize was implemented be Eugenie when he created MouseDirectManipulation.

Please review the committed revision 11242 that should fix the web client usage and the swing client is still working properly. I am not satisfied with this solution but there were issues related to hover regions when the mouse pointer during the resize operation enters or exits the existing hover region, then the resize mouse pointer can be broken by this logic in the web client. For these issues I used MouseDirectManipulation.this.updateBrowseWidgetMousePtr(boolean restore).

Stanislav Lomany wrote:

Please review the committed revision 11242 that should fix the web client usage and the swing client is still working properly.

The pointer is invalid when it is over the middle of a column:

Stanislav, I should switch to #3565. Please improve it if you have a vision of how it can be fixed, otherwise leave them as they are now.

I have to finish enhanced browse stuff first, so will see who will be available earlier.

Stanislav, see 3507a rev 11332 for another example how to create a node in the directory.xml , to set CURRENT-LANGUAGE (EnvironmentOps.setCurrentLanguage).

Stanislav, see 3507a rev 11332 for another example how to create a node in the directory.xml , to set CURRENT-LANGUAGE (EnvironmentOps.setCurrentLanguage).

Thank you, that did come in handy.

Is the state saving complete now?

Can we set a default in the directory that is overridden when a user makes changes?

Are you working on the hyperlinking?

Is the state saving complete now?

Still working on it.

Can we set a default in the directory that is overridden when a user makes changes?

You can see options in the menu above.

Are you working on the hyperlinking?

Not yet.

That is definitely pretty useful.

My only concern is that we should not give all users the ability to set defaults or override other user's choices. The options to save for groups, servers, all or default must only be allowed for someone with admin rights. I think an ACL is needed here. The server would have to check this on startup and inform the client so that the menus can be displayed appropriately.

You may be able to use the StringCondition* classes in com.goldencode.p2j.security.

Something else to add: simple users should have only a single option named "Save changes", without all the server/username specific info - this will save the settings at the user node, i.e. /server/<Servername>/runtime/<username>/. The complex options should be enabled only for specific users which have, as Greg mentions, special rights.

• boolean addDynamicFilter(FieldReference fr, BaseDataType val) - to add a filter for a field;
• boolean addDynamicSort(FieldReference fr, boolean asc) - to add a sorting criterion;
• void clearDynamicFilters() - clears all dynamic filters set for this query;
• void clearDynamicSorts() - clears all sorting criteria of the query.

The implementation is not affected for normal queries because the new dynamically added filters sort criteria are guarded by if-s. There can be a performance penalty because the HQL is re-processed (I don't think it's possible, in general, without it), but this will only happen for browsed queries. Since the UI is involved, I don't think the cost of the extra processing will be visible, except for really complex queries.

I tested these methods by calling them from a BrowseWidget method like this:

   public void addFilter(FieldReference fieldRef, BaseDataType val)
   {
      if (query == null)
      {
         return;
      }
      boolean wasOpen = query.isOpen().booleanValue();
      query.close();

      query.addDynamicFilter(fieldRef, val); /* replace with sort or clear methods */

      if (wasOpen)
      {
         query.open();
      }
   }

Eric,
Please note the changes in PreselectQuery:4240. I had to filter out some duplicate joins because they were returning the same rows multiple times if extent fields were involved in where predicate / sorting criteria.

Rebased task branch 3261c from P2J trunk revision 11253.

1. Incorrect enhanced popup menu for columns - fixed.
2. No stable reproduction for this so far:
   

   Caused by: java.lang.NullPointerException: Could not resolve window!
       at com.goldencode.p2j.ui.client.OutputManager.setInvalidate(OutputManager.java:1333)
       at com.goldencode.p2j.ui.client.gui.driver.GuiOutputManager.setInvalidate(GuiOutputManager.java:208)
       at com.goldencode.p2j.ui.chui.ThinClient.eventDrawingBracket(ThinClient.java:15764)
       at com.goldencode.p2j.ui.client.gui.SubMenuGuiImpl$SubMenuHideShowWorker.lambda$run$0(SubMenuGuiImpl.java:1747)
       at com.goldencode.p2j.ui.chui.ThinClient.waitForEvent(ThinClient.java:14704)
       at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:12434)
       at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:12009)
       at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:11952)
       at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:11906)
       ... 21 more
   

3. Has something changed in drawing? I call browse.repaint(), but it is not repainted: paint event is posted, but browse.draw() is not called.

Stanislav Lomany wrote:

After merging with the current trunk I met so far the following regressions:

1. No stable reproduction for this so far:
   [...]

This menu code has changed substantially in 3538c, which is soon to be merged to trunk. So your case will require a retest.

I call browse.repaint(), but it is not repainted: paint event is posted, but browse.draw() is not called.

repaint() is executed within invokeLaterOS, so I suppose that the paint event is trashed with the event queue created by invokeLaterOS. I can run

tc.postOSEvent(new PaintEvent(browse, browse.bounds()));

Stanislav Lomany wrote:

I call browse.repaint(), but it is not repainted: paint event is posted, but browse.draw() is not called.

repaint() is executed within invokeLaterOS, so I suppose that the paint event is trashed with the event queue created by invokeLaterOS.

Is the code checked in? What file and line number?

Is the code checked in? What file and line number?

private static MouseAdapter getColumnColorChangeAdapter(BrowseColumnGuiImpl column,
                                                           EnhancedBrowseParameter param)
   {
      return new MouseAdapter()
      {
         @Override
         public void mouseClicked(MouseEvent mouseEvent)
         {
            ThinClient tc = ThinClient.getInstance();
            tc.invokeLaterOS(() ->
            {
               ...
                  browse.repaint();
               }
            });
         }
      };
   }

1. Set enhanced-browse option in the directory:
   

   <node class="boolean" name="enhanced-browse">
      <node-attribute name="value" value="TRUE"/>
   </node>
   

2. Right-click a browse, select "Change Layout", "Change Browse Colors -> Text Color", select a color.
3. Nothing happens, but if you scroll the browse, it is redrawn with the new color.

Stanislav Lomany wrote:

Is the code checked in? What file and line number?

Yes: BrowsePopupMenu:435
[...]

Enclose browse.repaint(); in TC.eventDrawingBracket(). Also the invokeLaterOS() call in the handler will be redundant once 3538c is merged to trunk.

Rebased task branch 3261c from P2J trunk revision 11255.

I meant pop-up menus.

Also the invokeLaterOS() call in the handler will be redundant

Indeed, I got rid of it. repaint() also works fine without any extra wrapping.

Stanislav Lomany wrote:

Trunk has laggy drop-down menus. Do you see it?

Thanks for noting, I will look at this.

Hynek Cihlar wrote:

Stanislav Lomany wrote:

Trunk has laggy drop-down menus. Do you see it?

Thanks for noting, I will look at this.

It is caused by repaint() not issued for the lagging menu item.

Guys, this snippet

ds.openBatch(node);
ds.closeBatch(true);

[05/25/2018 12:13:23 MSK] (com.goldencode.p2j.security.SecurityManager:WARNING) Batch cannot be closed, it is not open for edit in the current security context.

The "security context" is just a reference to the user's session, it is not a reference to editing the security/ part of the directory.

At this point, can you just disable the editing code and simply read the directory for updates? We can manually edit the directory to customize the look and feel of the browse. If we can load those settings when present, then the most important feature will be achieved.

The user-defined edits and customization is nice to have, but it is not the most important thing. We can defer it. At this point we have sunk too much time in the saving/restoring of state and we simply must move on.

Rebased task branch 3261c from P2J trunk revision 11261.

From Stanislav:

Finished with saving enhanced configuration. There are some issues left:

1. Column attributes are applied to wrong column if there is a scrollable column under a locked column.
2. In the directory columns are keyed by converted field names rather than original field names.
3. Save options should be filtered regarding user's access rights.
4. Swing classic theme is redraw delay issue https://proj.goldencode.com/issues/3261#note-186 (Hynek involved).
5. Resize/drag issues in web version https://proj.goldencode.com/issues/3261#note-129 (Sergey involved).

I'm preparing the update for review. Which of the remaining issues should be resolved and when?

The most important feature of the visual customization is that we are able to have a set of defaults for all browses in the entire application (and all users). The idea is that customers are going to create a look/feel for the browse that should be a new default. The saving of more specific overrides is optional.

Can the current implementation support this?

Save options should be filtered regarding user's access rights.

Saving options should be off by default because no ACLs is the same as not having the rights to save. If we can do that, then we can manually save changes to the directory to set a system-wide default and the result will be OK for now.

The column settings are the part of the solution that is most important to enable proper saving of customization since this is not something that can be system-wide.

But we probably need to come back to this because we really must move on to other features. So it is important to make this safe and then finish it later.

Column attributes are applied to wrong column if there is a scrollable column under a locked column.

I'm not sure how important this is. If we are disabling user saving for now, then this can't be hit.

In the directory columns are keyed by converted field names rather than original field names.

This seems OK. What are the problems this causes?

Swing classic theme is redraw delay issue https://proj.goldencode.com/issues/3261#note-186 (Hynek involved).

You need to handle this, Hynek is busy.

Resize/drag issues in web version https://proj.goldencode.com/issues/3261#note-129 (Sergey involved).

I think Sergey is working on this today.

Can the current implementation support this?

Yes, override for all browses works.

Save options should be filtered regarding user's access rights.

But we probably need to come back to this because we really must move on to other features. So it is important to make this safe and then finish it later.

You can't save a configuration if don't have an access (according to general read/write directory ACLs): although extra save options are visually present, they return user-friendly error is you don't have an access. I don't think we should turn off saving because of this.

Column attributes are applied to wrong column if there is a scrollable column under a locked column.

I'm not sure how important this is. If we are disabling user saving for now, then this can't be hit.

I saw this in a customer GUI screen. This can be hit regardless of saving: it happens on the visual editing part.

In the directory columns are keyed by converted field names rather than original field names.

This seems OK. What are the problems this causes?

No problems, but I suppose for manual directory reading/editing original names are better (please correct me if don't think so).

Guys, could you advise on how I can set R+W access on the specific directory node (and subnodes)? This section doesn't seem to work. What am I missing? How directory access rights are defined when multiple access rules clash?

<node class="container" name="000700">
   <node class="strings" name="subjects">
      <node-attribute name="values" value="bogus"/>
   </node>
   <node class="directoryRights" name="rights">
      <node-attribute name="permissions" value="'00111111'B"/>
   </node>
   <node class="resource" name="resource-instance">
      <node-attribute name="reference" value="/server/standard/runtime/bogus/enhanced-browse-configs"/>
      <node-attribute name="reftype" value="TRUE"/>
   </node>
</node>

Guys, could you advise on how I can set R+W access on the specific directory node (and subnodes)?

Explicitly setting ACLs for account-specific nodes is a potential administrative burden that no customer will want to take on. I think there will be too many such paths to manage. Notice how the directory resources that are in the example directory files are more about managing access to the top level paths.

In our default directory, the admin will already have enough rights to store system-wide defaults.

This section doesn't seem to work. What am I missing?

I don't recall how the directory plugin works, but it may be disallowing lower resources if there are no rights to the top level parts of the paths.

How directory access rights are defined when multiple access rules clash?

Generally, when two rules clash, the first one in resource name order will "win". But the directory plugin may be special because it is meant to protect sections of the tree from access and/or modification and I am not sure if it is designed for the kind of overrides that you are doing.

You can't save a configuration if don't have an access (according to general read/write directory ACLs): although extra save options are visually present, they return user-friendly error is you don't have an access. I don't think we should turn off saving because of this.

As a general UI principle, users should not be shown anything as "enabled" and/or available if they actually don't have the rights to use it. This just invites them to do something that will fail, which is a possible source of frustration. Since we can know in advance if they have the rights to edit, then we can change the menu presentation.

Column attributes are applied to wrong column if there is a scrollable column under a locked column.

I'm not sure how important this is. If we are disabling user saving for now, then this can't be hit.

I saw this in a customer GUI screen. This can be hit regardless of saving: it happens on the visual editing part.

OK, then this needs to be fixed.

In the directory columns are keyed by converted field names rather than original field names.

This seems OK. What are the problems this causes?

No problems, but I suppose for manual directory reading/editing original names are better (please correct me if don't think so).

Yes, you are probably right. I think the common use case is to set system-wide defaults. More specific overrides are less likely to be used.

We are out of time on the saving work. If save is not complete as of last night, then please do the following:

1. Disable the menuing that allows saving.
2. Make any interrim code for saving safe for inclusion in the trunk.
3. Put TODOs into the code to explain where the code is not finished or not correct.

Make sure all of that is checked in to 3261c and then move on to the hyperlinking today.

I've made filtering of save option. The following permissions are checked for each of "enhanced-browse-configs" locations:

DirectoryService.canAccessNode(.../enhanced-browse-configs/some, 
                               DirectoryResource.DIR_WRITE_ACCESS,
                               DirectoryResource.DIR_CREATE_ACCESS)

<node class="container" name="000050">
   <node class="strings" name="subjects">
      <node-attribute name="values" value="adminuser"/>
   </node>
   <node class="directoryRights" name="rights">
      <node-attribute name="permissions" value="'00111111'B"/>
   </node>
   <node class="resource" name="resource-instance">
      <node-attribute name="reference" value=".*"/>
      <node-attribute name="reftype" value="false"/>
   </node>
</node>

I took a look at the enhanced menu issue for locked columns and it turned out to be a regression after which locked columns do not receive any mouse clicks at all which also affects editing etc.

revno: 11217 [merge]
committer: Greg Shah <ges@goldencode.com>
branch nick: trunk
timestamp: Fri 2018-02-02 10:45:48 -0500
message:
  This branch provides many fixes for a large GUI application, performance improvements, fixes related to supporting the new customer code base (which exposed from latent conversion issues), 4GL enhancements for URL-ENCODE and RT-OPSYS and the shifting of the Windows 8 theme into the main project (instead of as a plugin).  Refs #3435, #3369 (note 18), #3431, #3437, #3369, #3436, #3428, #3399, #3433, #3425, #3447, #3445, #3450, #3457, #3449).

Should I go for hyperlinking or for this?

Estimated effort to fix the mouse-clicks problem?

I'll answer later, I need to have a look what the issue is about.

OK, if it's more than 2-3 hours, switch to hyperlinking and we'll come back to this.

Please review 3261c rev 11278.

Fixed "non-clickable locked column set" regression. It is my regression because of a funny reason:

/**
 * Add the widget before the specified widget.
 * 
 * @param    before
 *           The widget before the new widget will be placed.
 * @param    widget
 *           The child widget to add.
 *
 * @return  this for fluent syntax support.
 * 
 * @throws  IllegalArgumentException
 *          when the argument is null or instance of {@link TopLevelWindow}
 */
@Override
public Container<O> add(Widget<O> before, Widget<O> widget)

Guys, about hyperlinking for editable browse: I believe we haven't settled a decision on how we can handle the case when clicking to a cell normally means the beginning of editing. I see the following options:
1. disable hyperlinking for editable columns.
2. perform default editing action on left click and hyperlinking is called thru right-click menu (or vise versa).
Your thoughts?

Stanislav Lomany wrote:

Guys, about hyperlinking for editable browse: I believe we haven't settled a decision on how we can handle the case when clicking to a cell normally means the beginning of editing. I see the following options:
1. disable hyperlinking for editable columns.
2. perform default editing action on left click and hyperlinking is called thru right-click menu (or vise versa).
Your thoughts?

How do you enter an editable cell to edit it? Via single click or double click? If is double-click, you may use single-click to open the hyperlink and double-click to open editable cell.

Constantin Asofiei wrote:

How do you enter an editable cell to edit it? Via single click or double click? If is double-click, you may use single-click to open the hyperlink and double-click to open editable cell.

In Windows standard feel, I think F2 is used to edit/rename things, but in ABL applications, usage of F2 might not be possible.

In 4GL single click starts editing, double click - starts editing and selects cell's content (not supported by P2J).

Stanislav Lomany wrote:

In 4GL single click starts editing, double click - starts editing and selects cell's content (not supported by P2J).

Considering that the enhanced browse is not a 4GL fully-compatible browse, we can make it work with click to open the hyperlink and double-click to edit.

Constantin Asofiei wrote:

Stanislav Lomany wrote:

In 4GL single click starts editing, double click - starts editing and selects cell's content (not supported by P2J).

Considering that the enhanced browse is not a 4GL fully-compatible browse, we can make it work with click to open the hyperlink and double-click to edit.

Also common key combination among applications is Ctrl+click to follow a hyperlink.

Also common key combination among applications is Ctrl+click to follow a hyperlink.

I like this option the most. Eric, Greg, please confirm.

Stanislav Lomany wrote:

Also common key combination among applications is Ctrl+click to follow a hyperlink.

I like this option the most. Eric, Greg, please confirm.

Agreed. Let's go with that.

Guys, remembering the browse pointer issues we have in web version, what is considered to be the correct way of changing the mouse pointer over a hyperlink area? Put the code in MouseDirectManipulation.mouseMoved or it can be handled within BrowseColumnGuiImpl.mouseMoved?

Stanislav Lomany wrote:

Guys, remembering the browse pointer issues we have in web version, what is considered to be the correct way of changing the mouse pointer over a hyperlink area? Put the code in MouseDirectManipulation.mouseMoved or it can be handled within BrowseColumnGuiImpl.mouseMoved?

I would suggest to use the MouseHoverAction, see WindowGuiImpl.init.

Hynek Cihlar wrote:

Stanislav Lomany wrote:

Guys, remembering the browse pointer issues we have in web version, what is considered to be the correct way of changing the mouse pointer over a hyperlink area? Put the code in MouseDirectManipulation.mouseMoved or it can be handled within BrowseColumnGuiImpl.mouseMoved?

I would suggest to use the MouseHoverAction, see WindowGuiImpl.init.

Better yet, override the event handlers mouseEntered and mouseExited in the widget and set the mouse cursor from there. Just don't forget to register for the mouse events with AbstractWidget.mouseActions.

Hynek Cihlar wrote:

Better yet, override the event handlers mouseEntered and mouseExited in the widget and set the mouse cursor from there. Just don't forget to register for the mouse events with AbstractWidget.mouseActions.

I just realized you asked about browse columns. In that case the advices in notes #3261-214 and #3261-215 won't work.

I'm not sure whether the MouseDirectManipulation mechanism can handle this case. If not then the GuiWebDriver.registerMouseWidgets will have to be extended to support multiple regions per widget.

Ovidiu Maxiniuc wrote:

The query support for dynamic sorting/filter was committed to branch 3261c (rev 11244).
The API is found in QueryWrapper:

• boolean addDynamicFilter(FieldReference fr, BaseDataType val) - to add a filter for a field;
• boolean addDynamicSort(FieldReference fr, boolean asc) - to add a sorting criterion;
• void clearDynamicFilters() - clears all dynamic filters set for this query;
• void clearDynamicSorts() - clears all sorting criteria of the query.

When setting and clearing the filter/sort state, it wasn't obvious to me that this was always safe as implemented. We are re-using existing query objects. When they are executed, they set all kinds of internal state. But when dynamic filters and sort criteria are added or cleared, that state is not being reset -- only the dynamic state is being updated (plus some component state). But the rest of the internal query state is not touched, AFAICT.

For example, in PreselectQuery, clearing the dynamic sort iterates through the query components and delegates the clear operation to them. But doesn't the state of the PreselectQuery itself need to be reset to its pristine state, so it re-executes the original query cleanly, as if it were new when the browse widget requests its rows after clearing a dynamic sort? Likewise, AdaptiveQuery maintains all kinds of crazy state, all of which I think would need to be reset to its defaults before changing where or order by clauses and re-running.

The implementation is not affected for normal queries because the new dynamically added filters sort criteria are guarded by if-s. There can be a performance penalty because the HQL is re-processed

You mean only in the case where the dynamic feature actually is used, right? This re-processing is avoided in the common case (no dynamic filter/sort)?

(I don't think it's possible, in general, without it), but this will only happen for browsed queries. Since the UI is involved, I don't think the cost of the extra processing will be visible, except for really complex queries.

I tested these methods by calling them from a BrowseWidget method like this:
[...]

Eric,
Please note the changes in PreselectQuery:4240. I had to filter out some duplicate joins because they were returning the same rows multiple times if extent fields were involved in where predicate / sorting criteria.

Makes sense.

Eric Faulhaber wrote:

When setting and clearing the filter/sort state, it wasn't obvious to me that this was always safe as implemented. We are re-using existing query objects. When they are executed, they set all kinds of internal state. But when dynamic filters and sort criteria are added or cleared, that state is not being reset -- only the dynamic state is being updated (plus some component state). But the rest of the internal query state is not touched, AFAICT.

In my code posted in note 174 the sort/filter updates are guarded by close() and open() operations on the wrapper. Isn't that enough to reset the internal state of all its components?

For example, in PreselectQuery, clearing the dynamic sort iterates through the query components and delegates the clear operation to them. But doesn't the state of the PreselectQuery itself need to be reset to its pristine state, so it re-executes the original query cleanly, as if it were new when the browse widget requests its rows after clearing a dynamic sort? Likewise, AdaptiveQuery maintains all kinds of crazy state, all of which I think would need to be reset to its defaults before changing where or order by clauses and re-running.

For AdaptiveQuery I found that changes in the super class combined with reopening the query seem enough to reset its 'crazy state'.

The implementation is not affected for normal queries because the new dynamically added filters sort criteria are guarded by if-s. There can be a performance penalty because the HQL is re-processed

You mean only in the case where the dynamic feature actually is used, right? This re-processing is avoided in the common case (no dynamic filter/sort)?

Yes, sorry for the incomplete phrase. In other words, if no dynamic features (sort/filters) are used (common case), the performance of queries is not affected at all.

The revision that you reviewed (there are a couple of bugs I fixed) tried not to affect the structure of the complex queries that was generated at runtime. Instead it attempted to work around it and enrich the where predicate or sorting criteria (or both in some cases) so that the final order of the rows to be changed or to select a sub-set of them. While for simple queries and filtering the implementation is fine, I discovered that for sorting of complex queries to work correctly we need to reconstruct the whole query in a different way that it was generated at conversion time.

Lets's take the following relatively simple example:

DEFINE QUERY q FOR tt, ss SCROLLING.
OPEN QUERY q FOR 
    EACH tt,
    FIRST ss WHERE tt.f1 EQ ss.g1
               AND tt.f3 EQ ss.g2
    BY tt.f2 DESC. 

query0.assign(new CompoundQuery().initialize(true, false));
query0.addComponent(new PreselectQuery().initialize(tt, ((String) null), null, "tt.f2 desc"));
query0.addComponent(new RandomAccessQuery().initialize(ss, "? = ss.g1 and ? = ss.g2", null, "ss.id asc", new Object[]
{
   new FieldReference(tt, "f1"),
   new FieldReference(tt, "f3")
}), QueryConstants.FIRST);

To sort by ss.g1 field of the inner table, the current solution searches for the component that iterates the ss table and add with higher priority "ss.g1 asc" to its sorting order. Well, this is not correct. Instead of changing the global order of the rows, it will change the order in which the FIRST ss is looked up, changing in this way the result set. Because of a relatively large result set (I started with the testcase from note 52 and adapted to task's necessities), I was not able to see that at the beginning.

To have a correct sorting the whole query wrapper need to be rebuilt. The easy way to see how its structure should look is to reconvert the testcase above using the additional sorting criterion. We get:

query1.assign(new PreselectQuery().initialize("ss.g1 asc"));
query1.addComponent(tt, ((String) null));
query1.addComponent(ss, "tt.f1 = ss.g1 and tt.f3 = ss.g2");
query1.setIterationType(QueryConstants.FIRST, "ss.id asc");

This is indeed a bit different. I tried to save somehow to save the query wrapper completely and create a new one with the new structure but it is a bit difficult, even to the simpler cases like this one.

I am thinking now of another solution: to grab all results from SQL server and sort them locally (client side). This will have an immense performance hit (time & memory) but I don't see for the moment a better solution.

Ovidiu Maxiniuc wrote:

I am thinking now of another solution: to grab all results from SQL server and sort them locally (client side). This will have an immense performance hit (time & memory) but I don't see for the moment a better solution.

Ugh. It's the memory I'm worried about. We could add something to the UI to allow a user to "kill" a query that was taking too long, but I don't think that's enough. If there is a very large result set backing the browse, it ALL would need to be pulled back to the application server for such a sort. Unlike other pre-sort queries where the size of the results would have been taken into account by the original 4GL developer, queries backing a browse might be associated with very large result sets, but today only a little is brought back at a time to populate a browse, so it is generally safe. If we were to now begin bringing ALL results back for such a sort, an unsuspecting user could bring the server down with an OOME. That's not an acceptable risk.

I'm not sure what the right answer is here. Any thoughts on how we could mitigate this risk?

I don't know how easy is to stop a "grabbing" process once it started on main processing thread. I don't see a secondary thread a solution - this will only complicate things. And another issue: what do we display in browse when the grab'n'sort process is stopped? incomplete data? nothing? revert to previous filter/sort state?

Ovidiu Maxiniuc wrote:

I don't know how easy is to stop a "grabbing" process once it started on main processing thread. I don't see a secondary thread a solution - this will only complicate things. And another issue: what do we display in browse when the grab'n'sort process is stopped? incomplete data? nothing? revert to previous filter/sort state?

Agreed, it's not a real solution, both for the reasons you state here and because it doesn't solve the more serious memory problem anyway.

It may be that for these types of queries specifically, we have to set a maximum number of records to retrieve and bail with an error message (e.g., "Too many rows to re-sort") if the result set we are trying to bring back to the application server exceeds that. At that point, I guess we have to revert to whatever the previous sorting was. We'll need a way to notify the browse widget that such a reset is necessary; probably a specialized specialized subclass of PersistenceException will do.

All FWD legacy queries going to a browse go through the Persistence.scroll API. The number of records brought back at a time from the database to the application server is set in the directory (hibernate/jdbc/fetch_size). So, we have control over how much we read before we decide it's a potential problem.

CompoundQuery gets more complicated, because each of its component queries is subject to this limit. Also, not sure how to handle queries which already use client-side filtering, where we potentially have to bring back a lot of records to filter down to a few. In this case, if the user knows there aren't a lot of rows in the browse to begin with, a "too many rows" error would seem weird. But IIRC, in that case we don't hold onto the raw records, we just run them through the filter and release them, so the issue here probably is not memory, but just the response time.

It's not ideal, but this scenario (FIRST/LAST component plus very large overall result set) probably is not the common case, so I'm thinking this could be a good compromise. We have freedom to choose the best implementation for the requirement, since we're not trying to match any legacy behavior.

We'll need a reliable, straightforward mechanism to know when we should go down this special logic path, and when the original implementation will work.

Thoughts?

Greg, in order to properly implement mouse hover over a hyperlink, it is far better to move all mouseMove code into one place: MouseDirectManipulation. I'm going to take Sergey's update, fix what is necessary and add hyperlinking.
Sergey, let me know if you've added anything except what is already committed as 3261c rev 11265.

Stanislav, I worked with rev 11265 in order to fix mouse pointer, but didn't succeed in this fixing. It seems we can continue this way if we fix a correct direction on the mouse movements.

Sergey, can I take over this task? It's blocking me (well, technically not, but I don't want to add more of bad design).

OK, please fix it.

Stanislav Lomany wrote:

Greg, in order to properly implement mouse hover over a hyperlink, it is far better to move all mouseMove code into one place: MouseDirectManipulation. I'm going to take Sergey's update, fix what is necessary and add hyperlinking.
Sergey, let me know if you've added anything except what is already committed as 3261c rev 11265.

My concerns are related to the following:
1. Please explain in details what do you mean by "move all mouseMove code into one place". What classes will be affected in this change? All mouseMove() capable widgets and handlers?
2. Direct manipulation mode is not always active (and should not be because it is heavy load to CPU in general).
3. On the other hand the mouse hover over hyperlink must always be active.
4. So I see many regressions from this change.

Why you want to use mouseMove() to handle hover? Doesn't the mouseEnter()/mouseExit() fit better? The mouseMove() is 1. really high frequently generated events and 2. can be arbitrary missed by OS in some conditions (not big coord change or too fast moving,...).

FYI, found NPE in MouseDirectManuipulation in web version when enhanced menu is called by right-click when the cursor is over the row resize area. I'm going to fix it.

Eugenie, I'm speaking only about the browse widget. Right now a browse cell has 4 resize areas: top/bottom row resize areas which are handled by MouseDirectManipulation and left/right which are handled by BrowseColumnGuiImpl. So mouseMoved is called twice: for both classes. And mouse cursor state is managed twice. Now I need to add the 5th area for a hyperlink inside a cell.
Also there is column movement which is handled by BrowseColumnGuiImpl.
And web version problems from note 129.
What's your advise for hyperlinking, where it should reside in your opinion? How in the perfect code the functionality should be separated between classes?

Doesn't the mouseEnter()/mouseExit() fit better?

I need to address only a part of the cell.

Stanislav Lomany wrote:

...
What's your advise for hyperlinking, where it should reside in your opinion? How in the perfect code the functionality should be separated between classes?

What functionality you are going to implement? Can you tell in few words? If the mouse become over part of the cell (which part btw, one that has a text?) - some additional processing become available for left or right mouse button press, correct understanding? Or something else?

Stanislav Lomany wrote:

...
What's your advise for hyperlinking, where it should reside in your opinion? How in the perfect code the functionality should be separated between classes?

In a perfect world I think if the object has some feature that activates by some event - this object must wait/keep track on this event and do the work. So speaking about browse the browse column is an object that has feature to activate, right? So I would use BrowseColumnGuiImpl as a base. This pseudo widget(the real one is the BrowseGuiImpl) must check if the mouse become over it (or part of it). If it is difficult to implement - we could create another transparent widget that located over browse column. This additional object with position ad size of the event area can be attached to browse column and dedicated to only find out the respective mouse event and inform the browse column (or browse) about the feature to be activated. That's my vision of the implementation way.

What functionality you are going to implement? Can you tell in few words? If the mouse become over part of the cell (which part btw, one that has a text?) - some additional processing become available for left or right mouse button press, correct understanding? Or something else?

I need to change mouse pointer over the text part of a cell (representing a hyperlink). When it is clicked, hyperlink is opened.

It seems that direct manipulation is activated by AbstractWidget.this.afterConfigUpdateBase(C beforeUpdate)

      // register/deregister direct manipulation handlers
      // should work only for GUI code and not for space/skip widgets
      if (isDirectManipulationCapable())
      {
         if (isDirectManipulationRequired() && !directManipulationActive)
         {
            activateDirectManipulation();
         }
         else if (!isDirectManipulationRequired() && directManipulationActive)
         {
            deactivateDirectManipulation();
         }
      }

   protected boolean isDirectManipulationRequired()
   {
      return config().selectable;
   }

   /**
    * Register the specified widget as a container able to use direct manipulation via mouse.
    * 
    * @param    widget
    *           The widget object to register.
    */
   @Override
   public void registerDirectManipulation(Widget<GuiOutputManager> widget)
   {
      int wid = widget.getId() != null ? widget.getId().asInt() : -1;
      MouseDirectManipulation worker = new MouseDirectManipulation(widget);

      MouseHandler.registerWidgetAction(wid, worker);
   }

Stanislav Lomany wrote:

What functionality you are going to implement? Can you tell in few words? If the mouse become over part of the cell (which part btw, one that has a text?) - some additional processing become available for left or right mouse button press, correct understanding? Or something else?

I need to change mouse pointer over the text part of a cell (representing a hyperlink). When it is clicked, hyperlink is opened.

Then the target object can be a hyperlinked text attached to (or associated with) browse column (it has location and size to track the mouse). This is how I would start thinking on implementation.

Stanislav Lomany wrote:

I need to change mouse pointer over the text part of a cell (representing a hyperlink).

You need to register a specific area, to have a distinct mouse pointer. I think this is done by emulating a distinct widget for the HyperLink, which has as area the boundary of the cell's hyperlink text - this widget should be found by findMouseSource - thus it needs to be added to Browse widget as a child, on top of the BrowseColumn - so it can be found before the actual cell. Later on, the widget's mouse enter/exit will take care of popping/pushing the mouse pointer, but this will actually be done via MouseHoverAction on Swing; see EditorGuiImpl.createMouseHoverAction for an example.

Why we need to emulate this HyperLink widget: because this way everything else works automatically, i.e. Web and Swing.

When it is clicked, hyperlink is opened.

This has no place in MouseDirectManipulation or mouse hover event. This is done by HyperLink widget's mouseClicked.

Eugenie Lyzenko wrote:

Then the target object can be a hyperlinked text attached to (or associated with) browse column (it has location and size to track the mouse). This is how I would start thinking on implementation.

You are on the right track, we need a dedicated widget for this hyperlink, to be able to handle the mouse click, hover cursor, etc naturally and not with hacks in the BrowseColumn widget.

Eugenie, do you think it is a good idea to handle column and row resizing in one place in MouseDirectManipulation? What about column moving?

Sergey Ivanovskiy wrote:

It seems that direct manipulation is activated by AbstractWidget.this.afterConfigUpdateBase(C beforeUpdate)
[...]
and can be deactivated only if isDirectManipulationRequired() returns false
[...]
and
in AbstractGuiDriver we have this code
[...]
It looks like it always listens mouse events.

Not always, only when widget is SELECTABLE==TRUE (FALSE by default).

Constantin Asofiei wrote:

Why we need to emulate this HyperLink widget: because this way everything else works automatically, i.e. Web and Swing.

When it is clicked, hyperlink is opened.

This has no place in MouseDirectManipulation or mouse hover event. This is done by HyperLink widget's mouseClicked.

I think mousePressed() to be exact, not mouseClicked.

Stanislav Lomany wrote:

Eugenie, do you think it is a good idea to handle column and row resizing in one place in MouseDirectManipulation? What about column moving?

• column/row resize should not be 'live', should be done by i.e. moving a transparent bar, and when 'dropped', then you have the actual resize - otherwise I don't see how Web will work fast, it will not be able to handle all these drawing operations.
• for column movement, if there is no full redrawing of browse, Web might be OK.

Why these side-notes: 4GL-style widget movement in FWD is already working poorly in Web; at least for 4GL-style movement, we should allow the driver to draw the rectangle (showing the the widget's area) itself, without trips to the FWD client; I don't know what events are raised by 4GL during movement/resize, but if there is nothing intermediary, then everything should be handled by the driver, between the start/stop operation.

Stanislav Lomany wrote:

Eugenie, do you think it is a good idea to handle column and row resizing in one place in MouseDirectManipulation? What about column moving?

MouseDirectManipulation in general is a place to handle widget's direct manipulation events without strict widget specific. Is a column movement the direct manipulation related event? If yes - it can be handled alongside all other direct manipulation events.

I don't know what events are raised by 4GL during movement/resize, but if there is nothing intermediary, then everything should be handled by the driver, between the start/stop operation.

I don't think the 4GL allows movement or resize of a browse column. That is purely in our enhanced mode. So there should be no 4GL events generated.

I don't think the 4GL allows movement or resize of a browse column. That is purely in our enhanced mode. So there should be no 4GL events generated.

It actually allows. Dotted outline is drawn at the place of the new bounds while a column is resized or moved.

Stanislav Lomany wrote:

I don't think the 4GL allows movement or resize of a browse column. That is purely in our enhanced mode. So there should be no 4GL events generated.

It actually allows. Dotted outline is drawn at the place of the new bounds while a column is resized or moved.

So what you're saying is you are moving bounds, and when you drop it, then the column is actually resized/moved? Can you post a screenshot from 4GL how this is done?

If this is the case, then MouseDirectManipulation is the place for this, so that when we push the movement/resize to the driver, everything will work the same.

Stanislav Lomany wrote:

Yes. 4GL documentation explicitly states that browse column resize/move is a direct manipulation event.

What doc says this? The Direct manipulation events table tells START-ROW-RESIZE/END-ROW-RESIZE are direct manipulation events specific to Browse widget (not browse column). And nothing about browse column move. This is DVREF.PDF doc for 1.1.7 version.

What doc says this?

Procedure editor help from the VM.

START-MOVE ... Frame and field-level widgets with MOVABLE attribute set to TRUE; for multiple widgets, SELECTABLE attribute also set to TRUE; Also browse-columns.

START-RESIZE ... Frame and field-level widgets with RESIZABLE and SELECTABLE attributes set to TRUE; Browse columns.

Ovidiu, please provide a status update on your part of this task. What is implemented, what is left? Is anything blocking you?

Eric Faulhaber wrote:

Ovidiu, please provide a status update on your part of this task. What is implemented, what is left? Is anything blocking you?

At this moment I am working on fixing the dynamic support. The idea seems simple, we need a client-side presorter to do the sorting on client side before sending the result to the browse. The good part of this solution is that PresorterDelegete accepts any kind of Resolvable not only simple fields.

• simple one: the delegate is already a PresortDelegate instance. In this case we just need to add the new Resolvable with higher priority;
• another simple one: the RandomAccessQuery. In this context, this will provide only one record. We don't need to do anything;
• in the cases of CompoundQuery and PreselectQuery things get messy. The solution I am trying to implement is to re-delegate the calls through new objects: PresortCompoundQuery and PresortPreselectQuery respectively. I am in front of a puzzle here: the pieces do not always fit together perfectly. I tried to re-build the wrapper by adding the existing compounds but it didn't work. For the moment my source code contains a lot of commented code from the previous unsuccessful attempts. I even considered writing (new ?) adapters that take CompoundQuery and PreselectQuery and make them act as presort counterparts.

My initial (wrong) approach was keeping the components as they were initialized at the beginning. The filtering was quite independent from sorting. When I will have the sorting working I need to test the filtering and combinations of these two primitives because most likely new objects will be constructed so they must keep track of the other dynamic activities from UI.

Rebased task branch 3261c from P2J trunk revision 11264.

Stanislav Lomany wrote:

1. Text width is not calculated properly.

I assume right-side is with Swing - the font for Web looks incorrect. What font are you using? How is the text width computed - via AbstractWidget.getTextWidthNative?

1. Does web version support underlined fonts?

Not yet. This is an outstanding issue - I think we need to manage it in p2j.screen.js, for the DRAW_STRING and DRAW_STRING_SCALED, to explicitly underline the text if the current font's style is underline - see p2j.fonts.isUnderlined.

1. MouseHoverAction doesn't work.

You mean it works in Swing but not in Web? How do you handle the link - is it via an explicit widget? For web, see GuiWebDriver.registerHoverableWidget and GuiWebDriver.registerMouseWidgets(), which takes care of informing the JS driver of widget boundaries which need explicit mouse actions.

What font are you using?

That is some arbitrary font set as the enhanced font. But the default font has this problem too (less obvious though).

How is the text width computed - via AbstractWidget.getTextWidthNative?

Yes. I've looked how it works: it calls SwingFontMetrics.stringWidth. Do we support web string width at all?

If my vision is correct, then all label strings used in application should be calculated against all configured fonts using special 4GL script and then these metric results should be written into text-metrics.xml. For an example, Hotel_GUI application uses this file to calculate all its label widths.

If my vision is correct, then all label strings used in application should be calculated against all configured fonts using special 4GL script and then these metric results should be written into text-metrics.xml. For an example, Hotel_GUI application uses this file to calculate all its label widths.

I don't care about labels. I need to calculate actual text with for an in-cell hyperlink in order to determine the effective area of the hyperlink. Any ideas are welcome.

We solved the similar issue when we highlight the web text, there is this method in AbstractGuiDriver

   public void drawTextSelection(TextLineSelection textLine,
                                 TextLookAndFeel textLookAndFeel,
                                 int x,
                                 int y,
                                 int lineHeight,
                                 int fontDescent)

There is also PaintPrimitives.FILL_RECT_AROUND_TEXT, as an idea how it can be solved.

How is the text width computed - via AbstractWidget.getTextWidthNative?

Yes. I've looked how it works: it calls SwingFontMetrics.stringWidth. Do we support web string width at all?

We used to calculate font metrics in javascript, but it is very expensive to do that because it is a down call to JS and back. The use of Swing for the metrics was made for performance reasons and generally the result has been quite usable.

I think it is possible to use these operations PaintPrimitives.TEXT_SHIFT and PaintPrimitives.UNDO_TEXT_SHIFT. Please look at this implementation to apply this idea for links if it is possible. There is also PaintPrimitives.FILL_RECT_AROUND_TEXT, as an idea how it can be solved.

It seems that these are means to properly draw text selection taking into account the actual text width, however it doesn't allow to get this actual width back to Java.

As a thought: I can draw text without cutting, but the actual hover area will be less than the hyperlink width. Unfortunately for the font above it will be 25% difference, but for the default fonts difference can be no more than 10%.

Yes, we can't get these resolved text metrics back to java side. It seems that if we set a height of rectangle around the given text small enough, then it can be looked like underlined text.

It seems that these are means to properly draw text selection taking into account the actual text width, however it doesn't allow to get this actual width back to Java.

We still support the GuiDriver.getTextWidth() and GuiDriver.getTextWidths() which return back the number of pixels for the drawn text. I think this is what you are looking for. We still use this in fillin and editor to get the text drawing right, if I recall correctly.

Constantin?

Whatever you do should be designed for good performance. If you really must get actual font metrics from the web client (JS side), then it would be best to do that in bulk instead of thousands of trips to the browser.

We still support the GuiDriver.getTextWidth()

It works with the same result as getTextWidthNative.

We still use this in fillin and editor to get the text drawing right, if I recall correctly.

If I understand correctly, it is drawn perfectly (well, except text jitter when you adjust selection) because of the tricks Sergey mentioned.

Yes, we can't get these resolved text metrics back to java side. It seems that if we set a height of rectangle around the given text small enough, then it can be looked like underlined text.

Yes, that can work. But in the first place I need to determine metrics for the hyperlink hover area, i.e. hyperlink widget size.

We still support the GuiDriver.getTextWidth()

It works with the same result as getTextWidthNative.

I think the backing code is still there (in the javascript side and the web socket processing).

Greg Shah wrote:

It seems that these are means to properly draw text selection taking into account the actual text width, however it doesn't allow to get this actual width back to Java.

We still support the GuiDriver.getTextWidth() and GuiDriver.getTextWidths() which return back the number of pixels for the drawn text. I think this is what you are looking for. We still use this in fillin and editor to get the text drawing right, if I recall correctly.

No. The GuiWebSocket's getTextHeight and getTextWidth are still there, but not used by the web driver. In all cases (FILL-IN included), we currently use Swing to compute the metrics.

Stanislav Lomany wrote:

Yes, that can work. But in the first place I need to determine metrics for the hyperlink hover area, i.e. hyperlink widget size.

I still want to understand why the font differs in width on Web. Do you have the server/fonts/ folder with the .ttf files? Is your custom-fonts node in directory.xml?

As a matter of general usage, the text (and widget) metrics should be the same regardless of the driver being used.

I still want to understand why the font differs in width on Web. Do you have the server/fonts/ folder with the .ttf files? Is your custom-fonts node in directory.xml?

I put everything in and now the situation is opposite: estimated text width is MORE than the actual. It was LESS before.
FYI, I'll be away for a while today.

Ovidiu: In regard to #3261-249:

• What additional findings do you have since you posted those details?
• What aspects of the solution need some resolution? Perhaps discussing these here might help to break through on finding a workable approach.

I still want to understand why the font differs in width on Web. Do you have the server/fonts/ folder with the .ttf files? Is your custom-fonts node in directory.xml?

OK, I resolved the fonts issue, it was a configuration problem: eventually I copied directory fonts config from the hotel app and, as in hotel, removed server/fonts folder.

Nevertheless, at this point, if you select some non-conventional enhanced browse font, drawing of hyperlinks and right-align columns (non-hyperlink too) fails.

Perhaps we should restrict the selectable fonts to only those which are configured in FWD.

You mean it works in Swing but not in Web? How do you handle the link - is it via an explicit widget?

Yes.

For web, see GuiWebDriver.registerHoverableWidget and GuiWebDriver.registerMouseWidgets(), which takes care of informing the JS driver of widget boundaries which need explicit mouse actions.

It doesn't help. registerHoverableWidget is called, then it is JS part where probably the issue is.

Note that hovering doesn't work properly for the fill-in inside an editable browse (which should has typing cursor). 50/50 it doesn't work at all or works if mouse enters fill-in in horizontal direction (if you move mouse vertically thru the fill-in, cursor doesn't change).

Stanislav Lomany wrote:

Note that hovering doesn't work properly for the fill-in inside an editable browse (which should has typing cursor). 50/50 it doesn't work at all or works if mouse enters fill-in in horizontal direction (if you move mouse vertically thru the fill-in, cursor doesn't change).

Is GuiWebDriver.registerMouseWidgets() being called at all for this hyperlink widget? Please try to debug (use a single-row browse with a single hyperlink cell), and check if the hyperlink widget's boundary gets transferred to the JS side. If the hyperlink widget has all the mouse actions set (via mouseActions() method), then it should be picked up here.

See also MouseWidgetsEnumerator interface which may help to get all browse components explicitly.

Guys, how can get the abend log in the web version? I'm handling NPEs probably caused by the recent changes related to getActualBounds().

Web client stderr log is in deploy/client/.

1. position mouse cursor over a browse column;
2. press Esc to exit the procedure containing the current browse - browse and all columns are destroyed;
3. move mouse - abend happens in WebMouseHandler or SwingMouseHandler because MOUSE_EXITED event is generated for the destroyed column.

What is wrong in this sequence - that event is generated or that it is not properly handled? Can I do something in BrowseColumnGuiImpl.destroy() to fix the problem?

Is GuiWebDriver.registerMouseWidgets() being called at all for this hyperlink widget? Please try to debug (use a single-row browse with a single hyperlink cell), and check if the hyperlink widget's boundary gets transferred to the JS side. If the hyperlink widget has all the mouse actions set (via mouseActions() method), then it should be picked up here.

See also MouseWidgetsEnumerator interface which may help to get all browse components explicitly.

Nothing helps. Eventually I tried to make the whole browse or a browse column hoverable just by modifying getEffectiveMousePointer() - same result: works in swing, but not in web.

Stanislav Lomany wrote:

I found an interesting sequence which leads to abend:

1. position mouse cursor over a browse column;
2. press Esc to exit the procedure containing the current browse - browse and all columns are destroyed;
3. move mouse - abend happens in WebMouseHandler or SwingMouseHandler because MOUSE_EXITED event is generated for the destroyed column.

What is wrong in this sequence - that event is generated or that it is not properly handled? Can I do something in BrowseColumnGuiImpl.destroy() to fix the problem?

Can you try the following patch?

=== modified file 'src/com/goldencode/p2j/ui/client/gui/driver/swing/SwingMouseHandler.java'
--- src/com/goldencode/p2j/ui/client/gui/driver/swing/SwingMouseHandler.java    2018-05-30 19:25:46 +0000
+++ src/com/goldencode/p2j/ui/client/gui/driver/swing/SwingMouseHandler.java    2018-06-19 13:55:04 +0000
@@ -544,7 +544,7 @@
          widget = findMouseSource(e);
       }

-      if (widget == null || !(widget.isVisible() && widget.isDisplayed()))
+      if (widget == null || !(widget.isDisplayed() && widget.isVisible()))
       {
          return false;
       }

I did the same change:) I'm not sure how to fix the same problem in WebMouseHandler.

Stanislav, are the BROWSE mouse issues in Web still a problem? If so, I can take a look - please point me to a branch and a testcase I can use.

Greg Shah wrote:

Ovidiu: In regard to #3261-249:

• What additional findings do you have since you posted those details?
• What aspects of the solution need some resolution? Perhaps discussing these here might help to break through on finding a workable approach.

I committed today a new revision (11285) to 3261c. I improved a few testcases and cleaned up the code a bit of some dead-ends. Also the P2JQuery interface has changed a bit since I realized the sorting methods are not required all over the place, only in the implementations of PresortDelegate.

I create now a new QueryWrapper from the old one and replaced it in BrowseWidget. This way, the CompoundQuery and PreselectQuery queries are transformed into their presorted counterparts. Since the stricture does change a bit, my great concerns with this solution might not accommodate the filtering of rows. But for the moment I see they work together.

I don't have any clue about the performance for the moment, but I will add some constraints as we discussed above after everything works acceptable.

1. defaultMousePtr() must be used to get a mouse pointer for the widget, and NOT getEffectiveMousePointer. Note that BrowseGuiImpl and EntryFieldGuiImpl still use getEffectiveMousePointer - please change them to use defaultMousePtr.
2. MouseEvt.MOUSE_HOVERABLE must be registered via mouseActions().
3. BROWSE has a peculiarity where MOUSE_MOVED must be registered via mouseActions().

And another issue: gd.registerMouseWidgets(); should never be executed explicitly, mouse widgets are registered with the JS driver only when the client ends up in a event-waiting loop (i.e. next wait-for iteration).

The resize/drag issues in web client (see #3261-129) are still open, correct?

Are all other mouse-related issues complete at this time?

The resize/drag issues in web client (see #3261-129) are still open, correct?

Yes.

Are all other mouse-related issues complete at this time?

Yes.

Stanislav: Can you please summarize the work that is left for this task? Highlight the parts that can be merged into the trunk before Monday morning (June 25th).

Stanislav: Can you please summarize the work that is left for this task? Highlight the parts that can be merged into the trunk before Monday morning (June 25th).

1. Web fonts sizing issues. I looked closely and hyperlinks are still cut a bit in Chrome, but fine in Firefox.
2. Web fonts issues 2. I solved the underlining issue: turned out the font I used had an alias which was used for drawing. When I cleared the alias, the font became underlined, but it also became a bit smaller as well in Chrome and a bit larger in Firefox. In addition in Chrome it has hard sizing issues (text length determination).
3. You suggested reducing the number of supported enhanced fonts.
4. The resize/drag issues in web client (#3261-129)
5. GUI part for sorting and filtering. I don't know how ready the persistence part is.

I think we will be able to solve and check in font issues (1-3). Is there a "reference" browser or we support both of them?

Is there a "reference" browser or we support both of them?

We support both. We also support Safari and IE.

You suggested reducing the number of supported enhanced fonts.

This is only needed if it is required to solve the font sizing issues. Normally, we would not want to restrict the fonts.

Stanislav Lomany wrote:

I found an interesting sequence which leads to abend:

1. position mouse cursor over a browse column;
2. press Esc to exit the procedure containing the current browse - browse and all columns are destroyed;
3. move mouse - abend happens in WebMouseHandler or SwingMouseHandler because MOUSE_EXITED event is generated for the destroyed column.

What is wrong in this sequence - that event is generated or that it is not properly handled? Can I do something in BrowseColumnGuiImpl.destroy() to fix the problem?

Hynek, I've fixed the similar web issue which you've fixed for swing: events for non-displayed widgets are now dropped (see the attached diff).

Original stacktrace:
[...]
Let know if you see issues with this fix.

The fix I did was only for handling hoverable and movable widgets. The mouse inputs for these are handled differently than regular mouse events. Regular mouse events are posted by the drivers (Swing, Web) to the event queue and eventually handled in TC.processProgressEvent (see the line evt.setMouseSource(mouseSource);). I think that the check for widget being displayed (and visible?) should be placed here in TC. Two reasons: (1) This is common place for both drivers where mouse events are handled, so we get consistent behavior and (2) the displayed-check in WebMouseHandler before the event is posted to the event queue may be to soon - there still may be other events already enqueued that may make the subjected widget undisplayed.

I think that the check for widget being displayed (and visible?) should be placed here in TC.

But we have abend earlier, before getting to TC, aren't we?

Stanislav Lomany wrote:

I think that the check for widget being displayed (and visible?) should be placed here in TC.

But we have abend earlier, before getting to TC, aren't we?

True, I was thinking about different case, where the widget is checked inside TC. Your change seems to be right for the protection of the call to screenPhysicalLocation.

Rebased task branch 3261c from P2J trunk revision 11266.

Stanislav Lomany wrote:

I took a closer look at the font sizing differences. Here is an example with the default font: Lato 8 (in swing it is actually Lato 11 because it was scaled to match the screen resolution). Then upper text is swing which is calculated to have 30 px in width (I see the 31st pale pixel though). Then middle text is web and it is 32 px in width. And the bottom is web text cut to 30 px.

I think we will not be able to have 100% compatible fonts between Web and Swing, due to font rendering differences. But AFAIK Swing gives the most accurate metrics when compared to 4GL metrics.

Does that mean that because of slight deviations in font rendering we have to get the metrics from js?

As mentioned before, this is what we are trying to avoid, as trips to JS side to compute text metrics are very expensive. Please try changing the width formula for the Hyperlink widget to this width = FontManager.getFontWidth(window, font, driver) * charNo; (from TextGuiImpl.nativeWidth:431) - I know we may get longer widths for certain texts, but as long as we are OK on average, it should be fine.

The following is a summary of directory.xml changes for enabling enhanced browse.

In order to enable enhanced functionality set this under /server/default/ or /server/<server_name>/:

        <node class="boolean" name="enhancedBrowseEnabled">
          <node-attribute name="value" value="TRUE"/>
        </node>
        <node class="boolean" name="browseHyperlinkingEnabled">
          <node-attribute name="value" value="TRUE"/>
        </node>

In order to save visual changes in enhanced mode, add the following to the /security/acl/directory/:

   <node class="container" name="ACL_ID">
      <node class="strings" name="subjects">
           <node-attribute name="values" value="USERNAME"/>
      </node>
      <node class="directoryRights" name="rights">
          <node-attribute name="permissions" value="'00111111'B"/>
       </node>
       <node class="resource" name="resource-instance">
          <node-attribute name="reference" value="DIRECTORY_PATH_OR_SPEC_HERE"/>
          <node-attribute name="reftype" value="TRUE_IF_HARD_CODED_PATH_FALSE_FOR_SPEC"/>
       </node>
   </node>

This is an ACL for editing the directory. Change the values as follows:

• ACL_ID must be a text value that makes this node unique compared to all others in this security/acl/directory/ container. The id should be a 6-digit numeric value such as 000050. The entire section should be placed in the proper numeric order under the security/acl/directory/ container. Do NOT duplicate any ACL_ID values in that container.
• USERNAME is the FWD account or group name that is being granted these permissions. An operating system userid cannot be used here because this is the configuration for FWD security not operating system security. The special value all_others can be used to assign these permissions to all accounts that don't already have a preceding ACL for their FWD account or for one of the FWD groups to which their FWD account belongs.
• DIRECTORY_PATH_OR_SPEC_HERE specifies the location in the directory that can be edited. The lookup processing checks in multiple directory locations. Defaults can set and user or group specific values can override the defaults. For more details on the lookup processing, see Utils.findDirectoryNodePath().
  • It can be a hard coded path (e.g. /server/{default|<SERVER_ID>}/runtime/{default|<FWD_ACCOUNT_OR_GROUP_ID>}/enhanced-browse-config/. The use of default allows setting/editing te default value for all servers. The use of a specific SERVER_ID (e.g. standard) will limit the edits to that specific FWD server's values. The default or FWD account or group is used to qualify the specific location below the .../runtime/ which is being referenced. /server/default/runtime/default/enhanced-browse-config would be the location that sets the location for all servers and all users. This will be honored unless there is saved configuration values found at the account or group level.
  • It can be a wildcard specification. To enable all directory editing access you can use .*, this is dangerous, so only use it for testing! You could also specify /server/default/runtime/.*/enhanced-browse-config to allow editing of the settings for all users/groups.
• TRUE_IF_HARD_CODED_PATH_FALSE_FOR_SPEC is used to define the reftype. The value will depend on the value for reference. If reference was a hard coded path, then use TRUE. If reference was a wildcard specification, then use FALSE.

Use the right mouse press on the browse to get the context menu. Use "Change Layout" to edit the look and feel. If the ACLs are correct, then any values modified during Change Layout can be saved using the context menu. If you see no saving options, then the ACLs have not provided write access to the "enhanced-browse-configs" node under the specific branch for the current user.

The dynamic sorting implementation seems now stable and complete. Committed as revision 11291 in branch 3261c.
Please review.

This is the list of work remaining in this task, as I know it.

• hyperlinking
  • underline style not honored for font usage in web client
  • text width is calculated incorrectly in web client
  • NPE in MouseHoverAction in web client
• sorting/filtering
  • query hierarchy filtering support (sorting is already complete) - OM
  • browse UI implementation to allow the user to control the sorting/filtering
    • up/down arrows in column headers to select sort direction
    • filtering entryfield integrated into column header
  • 4GL syntax to disable this feature for a given column (allows developers to avoid columns that will cause performance or other issues)
• resolve the conflict between enhanced browse popup menus and the popup menus defined by the original 4GL application
  • this issue is described in #3462-17 through #3462-31
  • that large GUI application does have a context menu, but it is inherited from the browse parent hierarchy
  • after more consideration, I have decided that it is best to resolve this now
  • the upper left corner of the browse has no purpose, in enhanced mode it will be drawn as a gear icon instead of the 3D placeholder that exists there now
  • pressing the gear icon will cause the enhanced browse popup to display
  • the regular 4GL popup menu processing can remain for the rest of the browse (right click opens any browse-specific context menu and if none exists, then the parent hierarchy popup menu is opened)
• add additional visual customization
  • flag to disable drawing row separators
  • flag to disable drawing column separators
  • flag to enable using different background colors for alternating lines
  • expose all of the Change Layout capabilities through 4GL syntax (the idea is that developers can use these same features to implement this customization in their 4GL code)
• bugs
  • web column/row resize issues
  • csv output doesn't load via a helper application in browser and the actual contents do not display in the browser directly
  • the color and font choosing dialogs can only be brought up once in the web client, the second time hangs the UI (see #3640)
  • colors and fonts are not honored in the exported report
  • at one point in testing the features, there was an alert box presented that reported a problem trying to "save column changes", it is not clear why this was shown but it seems like a bug

Stanislav: Am I missing anything that you know of?

I have some other things in consideration but I will post them separately.

What is the effort needed to add support for additional cell-level widgets? For example, we might want to make an image widget something that was definable for a browse cell (e.g. VIEW-AS IMAGE ...).

What work is needed to do this?

What is the effort needed to add support for additional cell-level widgets? For example, we might want to make an image widget something that was definable for a browse cell (e.g. VIEW-AS IMAGE ...).
What work is needed to do this?

It shouldn't take too much effort, because in-browse toggle box has been already implemented, so it is not hard to add handling of an alternative widget in known places. Additionally VIEW-AS requires conversion changes. That should take around 1.5 weeks.
If the new widget can enlarge row height, that is an additional problem.
If the new widget can enlarge row height of a particular row, that is a big problem.

If the new widget can enlarge row height, that is an additional problem.

Yes, this will possibly be needed.

If the new widget can enlarge row height of a particular row, that is a big problem.

I don't think this is necessary.

I tried to duplicate #3640 but the browse did not let me adjust the layout. After some investigations I found out that there is an issue with EXTENT fields. When such a field is expanded, only the first component gets a valid ehColumnKey for example tt.f5 (I think it should be something like tt.f5[1]), the rest remaining empty ("", not null) which renders the whole EnhancedBrowseConfig key set invalid (see BrowseGuiImpl.java:4427).

the browse did not let me adjust the layout

Make sure you have made the ACL changes as described in #3261-296. Use bogus for USERNAME, .* for DIRECTORY_PATH_OR_SPEC_HERE and FALSE for TRUE_IF_HARD_CODED_PATH_FALSE_FOR_SPEC.

I did that. I am getting Cannot create unique key for this browse, enhanced mode is not available! because the backupState.getKey().isValid() returns false in BrowseGuiImpl.startLayoutMode(). This is because one of the ehColumnKeys is empty.
After I manually set the keys for expanded EXTENT field, the Browse allowed me to edit its layout (I was interested in colour changes) and save my changes. At next run, they were correctly loaded.

The problem is in BrowseWidget.enhancedInit(). The columnKey is computed as fieldReference.getFullFieldName(). For first component of my extent field (f5[1]), this is tt.f5. When a second component is processed (f5[2]), FieldReference.getFullFieldName() returns the same - tt.f5. There is a checkDuplicateColumnKey that will force the second column key to be the empty string. I added the index information in getFullFieldName and everything went smooth.

I committed this change (and others related to filtering) as revision 11292 of 3261c.

Note that in case of computed columns, a similar collision may also occur! For example the column tt1.ff1 + tt1.ff2 and column tt1.ff1 - tt1.ff2 will have the same key so one of them will be set to "", making the browse layout editor stop working for the same reason.

Note that in case of computed columns, a similar collision may also occur! For example the column tt1.ff1 + tt1.ff2 and column tt1.ff1 - tt1.ff2 will have the same key so one of them will be set to "", making the browse layout editor stop working for the same reason.

Please go ahead and fix this now.

Greg Shah wrote:

Please go ahead and fix this now.

• a missing field (directoryNodeId) in copy constructor of EnhancedBrowseConfig was also copied - from my quick analysis of the code this was not intentionally;
• computed and expression columns key names are now unique for each BrowseWidget. In the event of a collision a warning is issued and printed to log.

• the changes are only applied when the browse is scrolled, window is moved or otherwise a refresh is triggered;
• the "Label Background Color" is not applied (it seems to be saved in directory and in application, when editing it the second time, the color is already preselected). The background of column label is always white. The foreground is OK, though.

• the changes are only applied when the browse is scrolled, window is moved or otherwise a refresh is triggered;

Could you provide the reproduction? I don't see the issue.

• the "Label Background Color" is not applied (it seems to be saved in directory and in application, when editing it the second time, the color is already preselected). The background of column label is always white. The foreground is OK, though.

The issue applies to win8/win10 themes. I'll fix it.

Stanislav Lomany wrote:

• the changes are only applied when the browse is scrolled, window is moved or otherwise a refresh is triggered;

Could you provide the reproduction? I don't see the issue.

I mean, the colour (or font) is not applied once the selection dialog is accepted. The user (or at least me) need to click somewhere in the window to get the cells painted with the colour I have chosen.

• the "Label Background Color" is not applied (it seems to be saved in directory and in application, when editing it the second time, the color is already preselected). The background of column label is always white. The foreground is OK, though.

The issue applies to win8/win10 themes. I'll fix it.

OK, I understand. Sorry.

Ovidiu Maxiniuc wrote:

Stanislav Lomany wrote:

• the changes are only applied when the browse is scrolled, window is moved or otherwise a refresh is triggered;

Could you provide the reproduction? I don't see the issue.

I mean, the colour (or font) is not applied once the selection dialog is accepted. The user (or at least me) need to click somewhere in the window to get the cells painted with the colour I have chosen.

This looks like a missed repaint for the BROWSE or the color/font for the BROWSE is not applied in a eventDrawingBracket, or both.

Note that if a new font is selected, the whole layout needs to be recalculated since changing the font size will resize some widgets, too.

Ovidiu Maxiniuc wrote:

Note that if a new font is selected, the whole layout needs to be recalculated since changing the font size will resize some widgets, too.

You mean for our enhanced BROWSE, or for 4GL code in general?

Constantin Asofiei wrote:

Ovidiu Maxiniuc wrote:

Note that if a new font is selected, the whole layout needs to be recalculated since changing the font size will resize some widgets, too.

You mean for our enhanced BROWSE, or for 4GL code in general?

To be clear, 4GL doesn't relayout/resize/etc the widgets after a SYSTEM-DIALOG FONT x is executed, and there is a i.e. with frame f1 font x frame. And this affects unrealized widgets, too, i.e.:

def var ch as char.

form ch with frame f1.

on "1" anywhere do:
system-dialog font 6.

update ch with frame f2 font 6.
end.

update ch with frame f1 font 6.

Constantin Asofiei wrote:

Ovidiu Maxiniuc wrote:

Note that if a new font is selected, the whole layout needs to be recalculated since changing the font size will resize some widgets, too.

You mean for our enhanced BROWSE, or for 4GL code in general?

I noticed the column header almost doubled when I changed the font to cmex10 (this is a really strange font for me) even if the size remained at 8. But I think this also applies to other cases when the font chooser dialog is used.

I mean, the colour (or font) is not applied once the selection dialog is accepted. The user (or at least me) need to click somewhere in the window to get the cells painted with the colour I have chosen.

I've never seen this issue. Could you fix it (because you can see it)?

Note that if a new font is selected, the whole layout needs to be recalculated since changing the font size will resize some widgets, too.

It IS recalculated. Let me know if it is not for you.

I noticed the column header almost doubled when I changed the font to cmex10 (this is a really strange font for me) even if the size remained at 8.

That seem to be some mathematical font, I don't have it. Let me know if header size is wrong for a commonly used font.

Stanislav Lomany wrote:

I mean, the colour (or font) is not applied once the selection dialog is accepted. The user (or at least me) need to click somewhere in the window to get the cells painted with the colour I have chosen.

I've never seen this issue. Could you fix it (because you can see it)?

Done more tests and compare them. This only happens to Swing driver.

Note that if a new font is selected, the whole layout needs to be recalculated since changing the font size will resize some widgets, too.

It IS recalculated. Let me know if it is not for you.

As Constantin mentioned, it probably should not be, in fact.

I noticed the column header almost doubled when I changed the font to cmex10 (this is a really strange font for me) even if the size remained at 8.

That seem to be some mathematical font, I don't have it. Let me know if header size is wrong for a commonly used font.

This is not important, the dimension of the text can be modified naturally by selecting the point size.

Question: when you choose font/color for an enhanced BROWSE, you are not altering the FONT-TABLE fonts, correct? This is just a setting of font/color RGB per this BROWSER.

Question: when you choose font/color for an enhanced BROWSE, you are not altering the FONT-TABLE fonts, correct? This is just a setting of font/color RGB per this BROWSER.

Correct.

• the upper left corner of the browse has no purpose, in enhanced mode it will be drawn as a gear icon instead of the 3D placeholder that exists there now

I'm not sure where we can put a gear icon: the upper left corner contains browse title OR column label OR first row.

Do we want to have two global parameters that enable enhanced sorting and enhanced filtering for ALL browses?

Yes.

I assume that non-enhanced sorting should be disabled if enhanced sorting is enabled.

Do you mean the 4GL events that would be used to implement sorting in hand coded 4GL?

If so, yes.

Also, what to do for browses with no labels? Disable enhanced functionality or force labels?

I don't think we can be sure that there are valid labels to use. I think we disable the functionality.

do you what to disable enhanced filtering/sorting for specific browses and browse columns thru 4GL syntax or thru enhanced popup menu?

Through 4GL syntax. There should be an option for the disable on the entire browse as well as a per-column option.

Do we need cross-like button to reset filter for the column (see the picture)?

Yes, that is a good idea. Please ensure that the X button is within the border of the entry field. It can be attached to the edge. I just want to ensure that the button is visually "part of" the entry field.

I suppose that for non-filtered columns fill-ins should be empty. Note that "?" is displayed for unknown numbers in fill-ins, we should avoid that.

Yes and yes.

• NPE in MouseHoverAction in web client

This is fixed.

Stanislav: Am I missing anything that you know of?

• Repaint issue Ovidiu reported.
• Delayed repaint in popup menu in classical swing theme.
• Existing check on directory access rights is insufficient: we need to check for more rights.

the "Label Background Color" is not applied

Fixed.

Now I guess I know the reason why "underline" support was commented out in JS code. "underline" is not a conventional specification for CanvasRenderingContext2D.font and when a font with "underline" is set, actually the previous font is kept.

Stanislav Lomany wrote:

Now I guess I know the reason why "underline" support was commented out in JS code. "underline" is not a conventional specification for CanvasRenderingContext2D.font and when a font with "underline" is set, actually the previous font is kept.

Yes, we need to explicitly underline the text in the DRAW_STRING, DRAW_STRING_SCALED and DRAW_PARAGRAPH cases. As you are already on JS side, use JS to measure the text width and height, and determine where to draw the line.

Ovidiu: What is the status of the filtering work? It is my understanding that the sorting is complete but that the filtering was not done yet.

From Ovidiu:

I tested both filtering and sorting with same queries and they both worked. I believe I covered all possible combinations but exceptions may exist.

The only exception is the case of expressions. Since they are encoded as lambdas, I am unable to encode them into HQL predicate or sort criteria. I thought of it but the only solution seems to be on client-side, merging somehow with the WHERE predicate.

The only exception is the case of expressions. Since they are encoded as lambdas, I am unable to encode them into HQL predicate or sort criteria. I thought of it but the only solution seems to be on client-side, merging somehow with the WHERE predicate.

What do you mean by "expressions"? Are you talking about the end user writing an expression in the filter entry field of a given browse column? Or are you talking about how we handle lambdas in the original query?

Greg Shah wrote:

What do you mean by "expressions"? Are you talking about the end user writing an expression in the filter entry field of a given browse column? Or are you talking about how we handle lambdas in the original query?

I was meaning the lambdas from the original query. They are not very usual, but sometimes they are used in browse too.
The hand-written expressions are a new idea. I have never though of them before in relation to browse widgets. This will bring the enhanced browse much close to an Excel sheet.

I was meaning the lambdas from the original query. They are not very usual, but sometimes they are used in browse too.

What do you think the effort is to implement this? We need to get it done so that the UI side can be finished.

Also: do you expect any changes to the API or could Stanislav write the client/UI changes today?

The hand-written expressions are a new idea. I have never though of them before in relation to browse widgets. This will bring the enhanced browse much close to an Excel sheet.

We won't implement this right now.

Greg Shah wrote:

I was meaning the lambdas from the original query. They are not very usual, but sometimes they are used in browse too.

What do you think the effort is to implement this? We need to get it done so that the UI side can be finished.

I think these falls in three categories: Resolvable, Supplier<BaseDataType> and Function<Object[], BaseDataType> but the handling should be very similar. Some parts of the code already support them (like PresortDelegate supports sorting of Resolvable s) but others need support form the scratch. A rough estimation is about a week, but probably the larger percent of it is for analysing and finding solutions comparing to actually writing code.

Also: do you expect any changes to the API or could Stanislav write the client/UI changes today?

Stanislav, I already added the four methods I used for testing in BrowseWidget.java. These are addDynamicFilter(), clearDynamicFilters(), addDynamicSort() and clearDynamicSorts. The add methods have the first parameter an FieldReference. I think you can build the UI around these methods. If not, we can discuss and add parameters as needed. If something goes wrong or you notice that the sorting and/or filtering do not work as expected, don't hesitate to post the issue along with the query and set of operation you used.
I will also add the methods necessary for computed columns in my next commit.

I don't have a problem in calling down to the JS side to get exact measurements. My issue is that I don't want this to regress performance.

Create a "bulk metrics" API call for the GUI driver. The idea is to provide a large number of strings and get the metrics for all of them in one call. For the web GUI driver, this would actually call down to the JS side and use the font metrics helpers that still exist. For now, this would only be used by browse.

Can the browse use case take this approach?

Can the browse use case take this approach?

Yes, sure, one js round trip for each new set of rows to display.

Stanislav Lomany wrote:

Can the browse use case take this approach?

Yes, sure, one js round trip for each new set of rows to display.

Please make sure metrics cache is in effect to reduce unnecessary trips to JS.

Do we want by default render right-aligned columns (not hyperlinks) using proper web metrics?

Stanislav Lomany wrote:

Do we want by default render right-aligned columns (not hyperlinks) using proper web metrics?

Yes. All browse text should be rendered properly.

Rebased task branch 3261c from P2J trunk revision 11272.

Please review 3261c rev 11304.

Please review 3261c rev 11304.

Added a fix: review 11305.

Code Review Task Branch 3261c Revision 11305

Overall this is a very good set of changes. It is so large that there are definitely portions which I have only reviewed quickly. I also know that we need to make some further changes in the directory save/restore portions BUT we will not do that in this branch.

I've asked Eric and Constantin to do code reviews as well.

Stanislav

1. What is the value for BrowseConfig.ehKeyBrowseName when using a dynamic browse?

2. I think we should implement HYPERLINK as a browse column attribute in addition to the widget option approach currently implemented.

3. I think that BrowseWidget.checkDuplicateColumnKey() should use a Set to quickly detect if there are conflicts. This should be much faster than the loops inside loops (checkDuplicateColumnKey() has a loop and is called from locations that are looping).

4. In EnhancedBrowseConfigManager.enhancedSaveTargets.initialValue(), where does the enhanced-browse-configs get added to the testNode? The comment states test arbitrary sub-node under .../enhanced-browse-configs. Either the comment is wrong or the code is wrong.

5. I love the LogicalTerminal.getBrowseById() addition!

Ovidiu

1. Do we need to provide a method for the 4GL developer to disable filtering (or even sorting) on a per-column basis? If there are columns that the developer definitely knows should not be used for filtering, this could allow them to eliminate any problems in advance.

2. AdaptiveComponent.addDynamicSortCriterion() needs javadoc.

A quick note: this code in RandomAccessQuery will fail first time the user inputs a single quote in the value: filterStr = fr.toString() + " = '" + val.toStringMessage() + "'";. Plus is vulnerable to SQL injection.

1. What is the value for BrowseConfig.ehKeyBrowseName when using a dynamic browse?

2. I think we should implement HYPERLINK as a browse column attribute in addition to the widget option approach currently implemented.

Currently hyperlink is client-side only widget. If you want to access widget attributes like hyperlink color (unless you what it to make derived from the cell foreground color), I suppose a server-side part should be created before we run ROW-DISPLAY trigger on the server side, and hyperlink attributes will be accessible in ROW-DISPLAY trigger. Should I do something on that part now?

3. I think that BrowseWidget.checkDuplicateColumnKey() should use a Set to quickly detect if there are conflicts. This should be much faster than the loops inside loops (checkDuplicateColumnKey() has a loop and is called from locations that are looping).

OK.

4. In EnhancedBrowseConfigManager.enhancedSaveTargets.initialValue(), where does the enhanced-browse-configs get added to the testNode? The comment states test arbitrary sub-node under .../enhanced-browse-configs. Either the comment is wrong or the code is wrong.

/enhanced-browse-configs can be found in EnhancedBrowseConfigManager.getDirectoryNode as ENHANCED_CONFIG_NODE constant.

Currently hyperlink is client-side only widget. If you want to access widget attributes like hyperlink color (unless you what it to make derived from the cell foreground color), I suppose a server-side part should be created before we run ROW-DISPLAY trigger on the server side, and hyperlink attributes will be accessible in ROW-DISPLAY trigger. Should I do something on that part now?

No, these are good ideas but they are not what I'm talking about.

Today, the code allows the 4GL developer to specify HYPERLINK "event-name" as a browse column option. This is implemented in frame_generator.rules.

I am asking for you to add a browse-column-widget:HYPERLINK attribute which when read would report "event-name" and when set would have the same effect as setting it as the widget option. It would be implemented in progess.g and in methods_attributes.rules. That is it.

Greg Shah wrote:

1. Do we need to provide a method for the 4GL developer to disable filtering (or even sorting) on a per-column basis? If there are columns that the developer definitely knows should not be used for filtering, this could allow them to eliminate any problems in advance.

I am thinking about this. The problem I see is that probably we should limit the columns. For the ABL programmer I think we can add an option FILTERABLE and SORTABLE when the column is defined. By default not active, to avoid negation in key-names. We add the sort/filter options in contextual menu of the column only if the respective option has been specified.

Constantin Asofiei wrote:

A quick note: this code in RandomAccessQuery will fail first time the user inputs a single quote in the value: filterStr = fr.toString() + " = '" + val.toStringMessage() + "'";. Plus is vulnerable to SQL injection.

Thank you, Constantin. I completely forgot about this. I improved the code by using a marshalling method in DbUtils. Please see r11307.

For the ABL programmer I think we can add an option FILTERABLE and SORTABLE when the column is defined. By default not active, to avoid negation in key-names.

I think it needs to active by default. Most columns should work and only those that would not process properly should be explicitly disabled.

We add the sort/filter options in contextual menu of the column only if the respective option has been specified.

This will not be a context menu. The sorting will be done with the up/down arrow in the header. The filtering will be done with an entry field in the column header.

Code review 3261c/11307 (persistence changes only):

The changes for the sort/filter feature set are less extensive than I thought would be needed. Nice work! Is this feature set now completely implemented from the persistence side?

Some comments:

• In AdaptiveComponent, please differentiate the javadoc for originalSort and originalStaticSort.
• Javadoc typo at AdaptiveQuery:2938 ("filed" -> "field"). Actually, I see this in other places (cut and paste, I guess), like CompoundQuery.addDynamicFilter.
• How is the optimization done in CompoundQuery managed w.r.t. dynamic filtering? You are augmenting the post-optimization components in addDynamicFilter, but you also added getOriginalComponents; where is that used?
• In FieldReference, you have added equals and hashCode implementations. In hashCode, you use Objects.hash(Object ...). It looks like this method would not be called very often (assuming QueryComponent.dynamicFilters is the only place it is used), so this is likely not an issue. However, please do not get in the habit of implementing hashCode with Objects.hash. Although it looks slightly neater, it creates an Object array and autoboxes any primitive value passed to it, which both add unnecessary overhead to the otherwise straightforward job of calculating a hash. For large maps, the calls to hashCode add up quickly. Generally speaking, I would rather we have a custom implementation that is more verbose, where we control the overhead ourselves.
• Please do not rewrite existing runtime internals to use lambda expressions, unless it is needed for some functional reason or performance benefit (example: Presorter.java:1093). If you haven't noticed, I am extremely sensitive to anything which might have a performance penalty. I realize this code is more compact using a lambda, but it is not clear what the performance implications of the change are, and I'm not willing to take the trade-off. I would rather be more conservative, more verbose, and not have to find new performance bottlenecks later.
• It looks like QueryComponent.prepareHQLPreprocessor is still vulnerable to SQL (HQL?) injection (see line 382).

2. I think we should implement HYPERLINK as a browse column attribute...
3. I think that BrowseWidget.checkDuplicateColumnKey() should use a Set to quickly detect if there are conflicts.

These are fixed in 3261c rev 11308.

In regard to revision 11307, I wonder if there are still ways that SQL injection can be done.

         // escape quote with two quotes, and wrap the result in quotes.
         return "'" + ((character) bdt).getValue().replaceAll("'", "''") + "'";

Could the character type contain escape chars or otherwise doubled/escaped quotes that could be used as an exploit?

I assume there are libraries that provide methods to detect SQL injection. Perhaps we should investigate these as an option.

      // other unknown BDTs  
      return "'" + bdt.toStringMessage() + "'";

Could longchar fall through here? It would be another type open to this kind of manipulation.

So would memptr, but we shouldn't be able to have a browse memptr column. Still, I would want us to put protection logic here for these cases even if we think they cant be hit. Changes in the future might miss the hidden dependency here and open up a latent security flaw.

Code Review Task Branch 3261c Revision 11308

Generally, I'm good with these changes. I've committed some minor changes as rev 11309.

1. The setHyperlinkEvent(character eventName) should have null and unknown value protection.

2. Please rebase the branch.

While we are waiting for Constantin's review, I think you can go ahead and start ChUI regression testing (conversion and runtime).

1. The setHyperlinkEvent(character eventName) should have null and unknown value protection.

I'll add null handling. Unknown/null value is valid: it turns off hyperlinking for this column.

Greg Shah wrote:

While we are waiting for Constantin's review

I don't see any other issues beside what was mentioned about DBUtils.toSqlValue.

Rebased 3261c from trunk rev 11273. Looking into merge issues.

Summary from Ovidiu about the remaining work needed for the query part of the filtering/sorting support:

There are two issues to be done:

• support for sort/filter of computed columns and
• support disabling filtering/sorting in P4GL Extension.

For 1st I already estimated about a week of work. I haven't started work here yet.
The 2nd I think can be implemented in conversion UI, no persistence involved.

Eric Faulhaber wrote:

The changes for the sort/filter feature set are less extensive than I thought would be needed. Nice work! Is this feature set now completely implemented from the persistence side?

The current revision is complete for field columns. The computed columns are not yet fully supported.

Some comments:

• In AdaptiveComponent, please differentiate the javadoc for originalSort and originalStaticSort.

Done.

• Javadoc typo at AdaptiveQuery:2938 ("filed" -> "field"). Actually, I see this in other places (cut and paste, I guess), like CompoundQuery.addDynamicFilter.

This is a personal typing defect. I try to fight it and correct when I see it but sometimes it just breaks free and because of ^C / ^V it spreads. With this occasion I searched and there are about a dozen other places in the project but I did not fixed in this revision.

• How is the optimization done in CompoundQuery managed w.r.t. dynamic filtering? You are augmenting the post-optimization components in addDynamicFilter, but you also added getOriginalComponents; where is that used?

The query is closed and re-opened each time the filtering is changed dynamically. The getOriginalComponents is used by QueryWrapper.createDynamicSorter() to created a PresortCompoundQuery in case the delegate is a CompoundQuery.

• In FieldReference, you have added equals and hashCode implementations. In hashCode, you use Objects.hash(Object ...). It looks like this method would not be called very often (assuming QueryComponent.dynamicFilters is the only place it is used), so this is likely not an issue. However, please do not get in the habit of implementing hashCode with Objects.hash. Although it looks slightly neater, it creates an Object array and autoboxes any primitive value passed to it, which both add unnecessary overhead to the otherwise straightforward job of calculating a hash. For large maps, the calls to hashCode add up quickly. Generally speaking, I would rather we have a custom implementation that is more verbose, where we control the overhead ourselves.

I agree. It was a temporary, quick solution. I replaced the hashing algorithm with a standard implementation based on prime numbers.

• Please do not rewrite existing runtime internals to use lambda expressions, unless it is needed for some functional reason or performance benefit (example: Presorter.java:1093). If you haven't noticed, I am extremely sensitive to anything which might have a performance penalty. I realize this code is more compact using a lambda, but it is not clear what the performance implications of the change are, and I'm not willing to take the trade-off. I would rather be more conservative, more verbose, and not have to find new performance bottlenecks later.

I understand. I just want to upgrade the code to latest Java version we use. I reverted the code.

• It looks like QueryComponent.prepareHQLPreprocessor is still vulnerable to SQL (HQL?) injection (see line 382).

Used DBUtils.toSqlValue in this case too. Note that I am using the Apache Commons' StringEscapeUtils.escapeSql() for string encoding. However, this does not fully protect the code from an SQL injection attack, but I an not sure there is a clean solution here taking into account that we are talking HQL and don't know the exact dialect involved. I also added some other changes to DBUtils.toSqlValue to avoid some issues described by Greg in note-351.

Committed to 3261c/11312.

Ovidiu Maxiniuc wrote:

• It looks like QueryComponent.prepareHQLPreprocessor is still vulnerable to SQL (HQL?) injection (see line 382).

Used DBUtils.toSqlValue in this case too. Note that I am using the Apache Commons' StringEscapeUtils.escapeSql() for string encoding. However, this does not fully protect the code from an SQL injection attack, but I an not sure there is a clean solution here taking into account that we are talking HQL and don't know the exact dialect involved. I also added some other changes to DBUtils.toSqlValue to avoid some issues described by Greg in note-351.

The cure to SQL injection generally is to use query substitution parameters instead of inlining. Have you considered the implications of this and rejected it as an approach? We already have the infrastructure to accept BDT subclass objects as substitution parameters, obviating the need for DBUtils.toSqlValue. I expect it is a more complicated and regression-prone approach, because of having to integrate with the existing parameters, re-indexing them, etc. However, it would be more secure in the end. Think about it and let me know your thoughts.

Eric Faulhaber wrote:

The cure to SQL injection generally is to use query substitution parameters instead of inlining. Have you considered the implications of this and rejected it as an approach? We already have the infrastructure to accept BDT subclass objects as substitution parameters, obviating the need for DBUtils.toSqlValue. I expect it is a more complicated and regression-prone approach, because of having to integrate with the existing parameters, re-indexing them, etc. However, it would be more secure in the end. Think about it and let me know your thoughts.

That was my first thought in fact. The introduction of a new set of parameters and reindexing the existing ones will affect the runtime for other queries than those browsed. Even if at first sight we just need to check their existence, we already do a similar thing for temp-tables, and things tends to become complicated.

I started to force this new idea in RandomAccessQuery and it seems to be easier now that I have the feature already implemented and I can focus only on this subtask. The good part is that the parameters for filtering may be more complex (computed columns ?) as the list for extra parameters can resolve all known types, like normal query substitutions.

I did not investigate at all the QueryComponent - the other place where the DBUtils.toSqlValue is used.

for some fonts size difference is pretty significant between web and swing, so I suppose we should limit the set of fonts OR let the administration select web or swing metrics as preferred

I thought you resolved this by using bulk web metrics when using a font override. Are you asking this because you are finding that even the bulk metrics cause a performance problem?

If this is the case, then when applying a font, how about measuring the differences between the web and swing font metrics? We can calculate if the difference is too much and automatically use the web metrics when needed. If the differences are small enough we continue to use the swing metrics.

I thought you resolved this by using bulk web metrics when using a font override.

I request web metrics only for hyperlinks and right-aligned columns. So far I was pretty happy with swing font height estimates, until I found several fonts that behave in this way.
I'll request font height when we need to recalculate row height after an arbitrary font is applied.

If this is the case, then when applying a font, how about measuring the differences between the web and swing font metrics? We can calculate if the difference is too much and automatically use the web metrics when needed. If the differences are small enough we continue to use the swing metrics.

Sounds good, will see how it works.

I missed the message that gc_64 fails in trunk, so consider the regression testing for 3261c passed. Should I merge it to trunk?

What testing has been done on the customer application? What about testing on Hotel GUI?

What testing has been done on the customer application? What about testing on Hotel GUI?

Some random testing in the customer app and hotel GUI.
BTW I've found an abend issue when layout is performed, bit it can be fixed in the next branch.

Stanislav Lomany wrote:

I missed the message that gc_64 fails in trunk, so consider the regression testing for 3261c passed. Should I merge it to trunk?

Stanislav,
I have some pending changes for dynamic filtering. They are needed because the client gets disconnected in some circumstances when filtering is active. I don't think the problems will surface if the dynamic filtering is not active, but if you have already tested then I can commit my changes to next branch (3261d).

Stanislav Lomany wrote:

What testing has been done on the customer application? What about testing on Hotel GUI?

Some random testing in the customer app and hotel GUI.

Please check the smoke tests in the customer app. If they work properly, then we can merge to trunk.

BTW I've found an abend issue when layout is performed, bit it can be fixed in the next branch.

Is this abend limited to enhanced mode?

Is this abend limited to enhanced mode?

Yes.

but if you have already tested then I can commit my changes to next branch (3261d).

I think that is a better option.

I understand. There a big set of changes and it is better to get it to trunk or else maintenance/rebasing is hell. If the regression tests were negative with r11316+ then merging to trunk should be done ASAP. We will fix these known issues in 3261d branch.

Ovidiu, I've tested only r11315 revision, so I'll restart regression testing. Let me know if you want to check in something today.

Guys, during the smoke test 1, when an estimate is added using the editable browse row, at the point just after a cell is updated, AbstractGuiDriver.ews is null, so I get NPE. Is it normal, that AbstractGuiDriver.ews is null?

Caused by: java.lang.NullPointerException
        at com.goldencode.p2j.ui.client.gui.driver.web.GuiWebDriver.cacheTextMetrics(GuiWebDriver.java:2365)
        at com.goldencode.p2j.ui.client.gui.BrowseGuiImpl.cacheTextMetrics(BrowseGuiImpl.java:2812)
        at com.goldencode.p2j.ui.client.gui.BrowseGuiImpl.updateColumn(BrowseGuiImpl.java:2792)
        at com.goldencode.p2j.ui.client.Browse.setColumnValue(Browse.java:4773)
        at com.goldencode.p2j.ui.client.BrowseColumn.setValue(BrowseColumn.java:315)
        at com.goldencode.p2j.ui.client.Frame.setValue(Frame.java:5483)
        at com.goldencode.p2j.ui.chui.ThinClient.setChangedValue(ThinClient.java:20329)
        at com.goldencode.p2j.ui.chui.ThinClient.refreshFrameWidget(ThinClient.java:11003)

Stanislav Lomany wrote:

Guys, during the smoke test 1, when an estimate is added using the editable browse row, at the point just after a cell is updated, AbstractGuiDriver.ews is null, so I get NPE. Is it normal, that AbstractGuiDriver.ews is null?
[...]

Whenever you are using the driver to get text metrics, you need to bracket the call in select/releaseWindow calls...

Please check the smoke tests in the customer app. If they work properly, then we can merge to trunk.

Greg, I've fixed all issues that were added by my changes.
You may want to review SwingMouseHandler in 3261c rev 11318 because it fixes a side issue.

Code Review Task Branch 3261c Revision 11318

Stanislav: The changes look fine to me.

Ovidiu: QueryComponent.resolveArg(), RandomAccessQuery.dynamicFilters, RandomAccessQuery.resolveArg() need javadoc.

Constantin/Sergey: Please review.

Eric: Please review the query changes.

Stanislav: Please restart ChUI regression testing (if you haven't already done so).

Am I correct that it has passed GUI testing (smoke tests in customer app and Hotel GUI)?

Stanislav: Please restart ChUI regression testing (if you haven't already done so).

I've tested 11317. 11318 contains only GUI changes.

Am I correct that it has passed GUI testing (smoke tests in customer app and Hotel GUI)?

Except "What if" button and smoke test 4 which don't work with trunk.

If the remaining code reviews are OK, then I would like you to merge to trunk.

Greg Shah wrote:

Code Review Task Branch 3261c Revision 11318
Ovidiu: QueryComponent.resolveArg(), RandomAccessQuery.dynamicFilters, RandomAccessQuery.resolveArg() need javadoc.

I addressed these issues in r11319. I tried to limit the changes to non-active code so that they won't affect the test results in any way.

Greg Shah wrote:

Eric: Please review the query changes.

Code review 3261c/11319 (persistence only):

• DBUtils.toSqlValue isn't referenced anymore, AFAICT. Can it be removed?
• RandomAccessQuery:3605: shouldn't we be dereferencing array index k instead of dLen for the currentArgs array?

Everything else looks good.

Eric Faulhaber wrote:

Code review 3261c/11319 (persistence only):

• DBUtils.toSqlValue isn't referenced anymore, AFAICT. Can it be removed?

The file was reverted to trunk revision.

• RandomAccessQuery:3605: shouldn't we be dereferencing array index k instead of dLen for the currentArgs array?

What a keen eye! Thanks for spotting this. Fixed.
Also the dynamicFilters was renamed to dynamicFilterArgs to better suit its function.

Current revision 11321.

Ovidiu Maxiniuc wrote:

Current revision 11321.

The changes from r11319 look good.

1. FontManager.registerFontDetails - you have selectWindow on line 412 instead of releaseWindow. Also, the releaseWindow call needs to be in a finally block.
2. GuiWebDriver.cacheTextMetrics - add the releaseWindow in a finally block.

Review for 3261c rev 11318:

1. FontManager.registerFontDetails - you have selectWindow on line 412 instead of releaseWindow. Also, the releaseWindow call needs to be in a finally block.
2. GuiWebDriver.cacheTextMetrics - add the releaseWindow in a finally block.

Thank you! Fixed.
Greg, I guess that is all. Can I commit?

BTW I think we should start a wiki page on enhanced functionality - how to enable etc.

I guess that is all. Can I commit?

How quickly can you test these changed paths using Hotel GUI? I just want to make sure the final code is safe.

If that is fine, then please do merge to trunk.

I think we should start a wiki page on enhanced functionality - how to enable etc.

I agree.

Put that content in Browse Widget Enhancements.

3261c has been merged into the trunk as bzr revision 11274.

After setting the enhancedBrowseEnabled in directory, the client got disconnected after right-clicking the header of a browse in customer's application. Here is the client-side stack trace listed in server's log:

Caused by: java.lang.IllegalStateException: Can not change window to -710 within the same thread, if a window is already selected!
        at com.goldencode.p2j.ui.client.gui.driver.AbstractGuiDriver.selectWindow(AbstractGuiDriver.java:2167)
        at com.goldencode.p2j.ui.client.gui.driver.GuiOutputManager.selectWindow(GuiOutputManager.java:502)
        at com.goldencode.p2j.ui.client.OutputManager.screenBitmap(OutputManager.java:1657)
        at com.goldencode.p2j.ui.client.gui.driver.GuiOutputManager.screenBitmap(GuiOutputManager.java:306)
        at com.goldencode.p2j.ui.client.OutputManager.getBitmapCopy(OutputManager.java:959)
        at com.goldencode.p2j.ui.client.gui.SubMenuGuiImpl.draw(SubMenuGuiImpl.java:669)
        at com.goldencode.p2j.ui.client.widget.AbstractContainer.draw(AbstractContainer.java:518)
        at com.goldencode.p2j.ui.client.gui.MenuGuiImpl.access$001(MenuGuiImpl.java:112)
        at com.goldencode.p2j.ui.client.gui.MenuGuiImpl.lambda$draw$1(MenuGuiImpl.java:367)
        at com.goldencode.p2j.ui.client.gui.driver.AbstractGuiDriver.draw(AbstractGuiDriver.java:2526)
        at com.goldencode.p2j.ui.client.gui.MenuGuiImpl.draw(MenuGuiImpl.java:367)
        at com.goldencode.p2j.ui.client.widget.AbstractContainer.draw(AbstractContainer.java:518)
        at com.goldencode.p2j.ui.client.gui.BorderedPanelGuiImpl.access$001(BorderedPanelGuiImpl.java:94)
        at com.goldencode.p2j.ui.client.gui.BorderedPanelGuiImpl.lambda$null$0(BorderedPanelGuiImpl.java:309)
        at com.goldencode.p2j.ui.client.gui.driver.AbstractGuiDriver.draw(AbstractGuiDriver.java:2526)
        at com.goldencode.p2j.ui.client.gui.driver.AbstractGuiDriver.draw(AbstractGuiDriver.java:2569)
        at com.goldencode.p2j.ui.client.gui.BorderedPanelGuiImpl.lambda$draw$1(BorderedPanelGuiImpl.java:295)
        at com.goldencode.p2j.ui.client.gui.driver.AbstractGuiDriver.draw(AbstractGuiDriver.java:2526)
        at com.goldencode.p2j.ui.client.gui.driver.AbstractGuiDriver.draw(AbstractGuiDriver.java:2569)
        at com.goldencode.p2j.ui.client.gui.BorderedPanelGuiImpl.draw(BorderedPanelGuiImpl.java:245)
        at com.goldencode.p2j.ui.client.widget.AbstractContainer.draw(AbstractContainer.java:518)
        at com.goldencode.p2j.ui.client.widget.OuterFrame.drawInt(OuterFrame.java:146)
        at com.goldencode.p2j.ui.client.widget.OuterFrame.draw(OuterFrame.java:138)
        at com.goldencode.p2j.ui.client.gui.OverlayWindow.draw(OverlayWindow.java:229)
        at com.goldencode.p2j.ui.client.OutputManager.setInvalidate(OutputManager.java:1373)
        at com.goldencode.p2j.ui.client.gui.driver.GuiOutputManager.setInvalidate(GuiOutputManager.java:210)
        at com.goldencode.p2j.ui.chui.ThinClient.eventDrawingBracket(ThinClient.java:16105)
        at com.goldencode.p2j.ui.chui.ThinClient.independentEventDrawingBracket(ThinClient.java:15963)
        at com.goldencode.p2j.ui.chui.ThinClient.processRepaints(ThinClient.java:14943)
        at com.goldencode.p2j.ui.chui.ThinClient.waitForEvent(ThinClient.java:15005)
        at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:12691)
        at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:12255)
        at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:12193)
        at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:12142)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:124)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:757)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:412)
        at com.goldencode.p2j.net.Conversation.waitMessage(Conversation.java:348)
        at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1186)
        at com.goldencode.p2j.net.Queue.transact(Queue.java:657)
        at com.goldencode.p2j.net.BaseSession.transact(BaseSession.java:271)
        at com.goldencode.p2j.net.HighLevelObject.transact(HighLevelObject.java:211)
        at com.goldencode.p2j.net.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1473)
        at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:145)
        at com.sun.proxy.$Proxy11.trigger(Unknown Source)
        at com.goldencode.p2j.ui.chui.ThinClient.trigger(ThinClient.java:13449)
        at com.goldencode.p2j.ui.chui.ThinClient.invokeTriggers(ThinClient.java:19234)
        at com.goldencode.p2j.ui.chui.ThinClient.invokeTriggers(ThinClient.java:19006)
        at com.goldencode.p2j.ui.chui.ThinClient.invokeTriggers(ThinClient.java:18892)
        at com.goldencode.p2j.ui.chui.ThinClient.processProgressEvent(ThinClient.java:18146)
        at com.goldencode.p2j.ui.chui.ThinClient.processEventsWorker(ThinClient.java:17195)
        at com.goldencode.p2j.ui.chui.ThinClient.pop(ThinClient.java:16192)
        at com.goldencode.p2j.ui.chui.ThinClient.eventBracket(ThinClient.java:16175)
        at com.goldencode.p2j.ui.chui.ThinClient.eventDrawingBracket(ThinClient.java:16099)
        at com.goldencode.p2j.ui.chui.ThinClient.applyWorker(ThinClient.java:15849)
        at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:12781)
        at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:12255)
        at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:12193)
        at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:12142)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:124)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:757)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:412)
        at com.goldencode.p2j.net.Conversation.waitMessage(Conversation.java:348)
        at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1186)
        at com.goldencode.p2j.net.Queue.transact(Queue.java:657)
        at com.goldencode.p2j.net.BaseSession.transact(BaseSession.java:271)
        at com.goldencode.p2j.net.HighLevelObject.transact(HighLevelObject.java:211)
        at com.goldencode.p2j.net.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1473)
        at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:145)
        at com.sun.proxy.$Proxy13.standardEntry(Unknown Source)
        at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:361)
        at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:158)
        at com.goldencode.p2j.main.ClientDriver.start(ClientDriver.java:250)
        at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:444)
        at com.goldencode.p2j.main.ClientDriver.process(ClientDriver.java:144)
        at com.goldencode.p2j.main.ClientDriver.main(ClientDriver.java:313)

Add com.goldencode.util.LogHelper.traces(15, windowId) to selectWindow() and find out the places that do window select. This should provide more info to figure out what is going on.

Hynek Cihlar wrote:

Add com.goldencode.util.LogHelper.traces(15, windowId) to selectWindow() and find out the places that do window select. This should provide more info to figure out what is going on.

I did. Where do I find the output for the web driver?
I tried to put a breakpoint and see manually but it just disconnected in two other places:

Caused by: java.lang.NullPointerException: Could not resolve window!
        at com.goldencode.p2j.ui.client.OutputManager.setInvalidate(OutputManager.java:1335)
        at com.goldencode.p2j.ui.client.gui.driver.GuiOutputManager.setInvalidate(GuiOutputManager.java:210)
        at com.goldencode.p2j.ui.chui.ThinClient.eventDrawingBracket(ThinClient.java:16105)
        at com.goldencode.p2j.ui.chui.ThinClient.eventDrawingBracket(ThinClient.java:16002)
        at com.goldencode.p2j.ui.client.gui.SubMenuGuiImpl.lambda$null$7(SubMenuGuiImpl.java:1421)
        at com.goldencode.p2j.ui.chui.ThinClient.waitForEvent(ThinClient.java:15028)
        at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:12691)
        at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:12255)
        at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:12193)
        at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:12142)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:124)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:757)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:412)
        at com.goldencode.p2j.net.Conversation.waitMessage(Conversation.java:348)
        at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1186)
        at com.goldencode.p2j.net.Queue.transact(Queue.java:657)
        at com.goldencode.p2j.net.BaseSession.transact(BaseSession.java:271)
        at com.goldencode.p2j.net.HighLevelObject.transact(HighLevelObject.java:211)
        at com.goldencode.p2j.net.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1473)
        at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:145)
        at com.sun.proxy.$Proxy11.trigger(Unknown Source)
        at com.goldencode.p2j.ui.chui.ThinClient.trigger(ThinClient.java:13449)
        at com.goldencode.p2j.ui.chui.ThinClient.invokeTriggers(ThinClient.java:19234)
        at com.goldencode.p2j.ui.chui.ThinClient.invokeTriggers(ThinClient.java:19006)
        at com.goldencode.p2j.ui.chui.ThinClient.invokeTriggers(ThinClient.java:18892)
        at com.goldencode.p2j.ui.chui.ThinClient.processProgressEvent(ThinClient.java:18146)
        at com.goldencode.p2j.ui.chui.ThinClient.processEventsWorker(ThinClient.java:17195)
        at com.goldencode.p2j.ui.chui.ThinClient.pop(ThinClient.java:16192)
        at com.goldencode.p2j.ui.chui.ThinClient.eventBracket(ThinClient.java:16175)
        at com.goldencode.p2j.ui.chui.ThinClient.eventDrawingBracket(ThinClient.java:16099)
        at com.goldencode.p2j.ui.chui.ThinClient.applyWorker(ThinClient.java:15849)
        at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:12781)
        at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:12255)
        at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:12193)
        at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:12142)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:124)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:757)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:412)
        at com.goldencode.p2j.net.Conversation.waitMessage(Conversation.java:348)
        at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1186)
        at com.goldencode.p2j.net.Queue.transact(Queue.java:657)
        at com.goldencode.p2j.net.BaseSession.transact(BaseSession.java:271)
        at com.goldencode.p2j.net.HighLevelObject.transact(HighLevelObject.java:211)
        at com.goldencode.p2j.net.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1473)
        at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:145)
        at com.sun.proxy.$Proxy13.standardEntry(Unknown Source)
        at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:361)
        at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:158)
        at com.goldencode.p2j.main.ClientDriver.start(ClientDriver.java:250)
        at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:444)
        at com.goldencode.p2j.main.ClientDriver.process(ClientDriver.java:144)
        at com.goldencode.p2j.main.ClientDriver.main(ClientDriver.java:313)

Caused by: java.lang.IllegalStateException: widget is not in the container
        at com.goldencode.p2j.ui.client.widget.AbstractWidget.screenPhysicalLocation_aroundBody2(AbstractWidget.java:1176)
        at com.goldencode.p2j.ui.client.widget.AbstractWidget.screenPhysicalLocation_aroundBody3$advice(AbstractWidget.java:241)
        at com.goldencode.p2j.ui.client.widget.AbstractWidget.screenPhysicalLocation(AbstractWidget.java:1)
        at com.goldencode.p2j.ui.client.widget.AbstractWidget.screenPhysicalLocation_aroundBody2(AbstractWidget.java:1180)
        at com.goldencode.p2j.ui.client.widget.AbstractWidget.screenPhysicalLocation_aroundBody3$advice(AbstractWidget.java:241)
        at com.goldencode.p2j.ui.client.widget.AbstractWidget.screenPhysicalLocation(AbstractWidget.java:1)
        at com.goldencode.p2j.ui.client.gui.driver.web.WebMouseHandler.lambda$raiseMouseEvent$1(WebMouseHandler.java:123)
        at com.goldencode.p2j.ui.chui.ThinClient.waitForEvent(ThinClient.java:15028)
        at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:12691)
        at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:12255)
        at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:12193)
        at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:12142)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:124)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:757)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:412)
        at com.goldencode.p2j.net.Conversation.waitMessage(Conversation.java:348)
        at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1186)
        at com.goldencode.p2j.net.Queue.transact(Queue.java:657)
        at com.goldencode.p2j.net.BaseSession.transact(BaseSession.java:271)
        at com.goldencode.p2j.net.HighLevelObject.transact(HighLevelObject.java:211)
        at com.goldencode.p2j.net.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1473)
        at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:145)
        at com.sun.proxy.$Proxy11.trigger(Unknown Source)
        at com.goldencode.p2j.ui.chui.ThinClient.trigger(ThinClient.java:13449)
        at com.goldencode.p2j.ui.chui.ThinClient.invokeTriggers(ThinClient.java:19234)
        at com.goldencode.p2j.ui.chui.ThinClient.invokeTriggers(ThinClient.java:19006)
        at com.goldencode.p2j.ui.chui.ThinClient.invokeTriggers(ThinClient.java:18892)
        at com.goldencode.p2j.ui.chui.ThinClient.processProgressEvent(ThinClient.java:18146)
        at com.goldencode.p2j.ui.chui.ThinClient.processEventsWorker(ThinClient.java:17195)
        at com.goldencode.p2j.ui.chui.ThinClient.pop(ThinClient.java:16192)
        at com.goldencode.p2j.ui.chui.ThinClient.eventBracket(ThinClient.java:16175)
        at com.goldencode.p2j.ui.chui.ThinClient.eventDrawingBracket(ThinClient.java:16099)
        at com.goldencode.p2j.ui.chui.ThinClient.applyWorker(ThinClient.java:15849)
        at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:12781)
        at com.goldencode.p2j.ui.chui.ThinClient.waitForWorker(ThinClient.java:12255)
        at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:12193)
        at com.goldencode.p2j.ui.chui.ThinClient.waitFor(ThinClient.java:12142)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at com.goldencode.p2j.util.MethodInvoker.invoke(MethodInvoker.java:124)
        at com.goldencode.p2j.net.Dispatcher.processInbound(Dispatcher.java:757)
        at com.goldencode.p2j.net.Conversation.block(Conversation.java:412)
        at com.goldencode.p2j.net.Conversation.waitMessage(Conversation.java:348)
        at com.goldencode.p2j.net.Queue.transactImpl(Queue.java:1186)
        at com.goldencode.p2j.net.Queue.transact(Queue.java:657)
        at com.goldencode.p2j.net.BaseSession.transact(BaseSession.java:271)
        at com.goldencode.p2j.net.HighLevelObject.transact(HighLevelObject.java:211)
        at com.goldencode.p2j.net.RemoteObject$RemoteAccess.invokeCore(RemoteObject.java:1473)
        at com.goldencode.p2j.net.InvocationStub.invoke(InvocationStub.java:145)
        at com.sun.proxy.$Proxy13.standardEntry(Unknown Source)
        at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:361)
        at com.goldencode.p2j.main.ClientCore.start(ClientCore.java:158)
        at com.goldencode.p2j.main.ClientDriver.start(ClientDriver.java:250)
        at com.goldencode.p2j.main.CommonDriver.process(CommonDriver.java:444)
        at com.goldencode.p2j.main.ClientDriver.process(ClientDriver.java:144)
        at com.goldencode.p2j.main.ClientDriver.main(ClientDriver.java:313)

The trigger is the right-click on the header of an enhanced browse. I don't get to click any item, just hovering over them and I can see the windows from background start to dissaper and I get the disconnect message. I haven't tried yet the swing driver, all these occurred with web driver.

Ovidiu Maxiniuc wrote:

Hynek Cihlar wrote:

Add com.goldencode.util.LogHelper.traces(15, windowId) to selectWindow() and find out the places that do window select. This should provide more info to figure out what is going on.

I did. Where do I find the output for the web driver?

The log file is written to the client process work dir, defined in directory.xml/../webClient/../workingDir.

Updated list of work remaining. Completed items are crossed out. Items not listed before are bold.

• hyperlinking
  • underline style not honored for font usage in web client
  • text width is calculated incorrectly in web client
  • NPE in MouseHoverAction in web client
• sorting/filtering
  • query hierarchy filtering support (sorting is already complete) - OM
  • browse UI implementation to allow the user to control the sorting/filtering
    • up/down arrows in column headers to select sort direction
    • filtering entryfield integrated into column header
  • 4GL syntax to disable this feature for a given column (allows developers to avoid columns that will cause performance or other issues)
• resolve the conflict between enhanced browse popup menus and the popup menus defined by the original 4GL application. The upper left corner of the browse has no purpose, in enhanced mode it will be drawn as a gear icon instead of the 3D placeholder that exists there now pressing the gear icon will cause the enhanced browse popup to display the regular 4GL popup menu processing can remain for the rest of the browse (right click opens any browse-specific context menu and if none exists, then the parent hierarchy popup menu is opened)
• add additional visual customization
  • flag to disable drawing row separators
  • flag to disable drawing column separators
  • flag to enable using different background colors for alternating lines
  • flag to disable Change Layout UI while still allowing any directory customization to be honored
  • rework of the Save Changes processing and directory approach for layout/UI customization as described in #3261-436
  • expose all of the Change Layout capabilities through 4GL syntax (the idea is that developers can use these same features to implement this customization in their 4GL code)
• bugs
  • web column/row resize issues
  • csv output doesn't load via a helper application in browser and the actual contents do not display in the browser directly
  • the color and font choosing dialogs can only be brought up once in the web client, the second time hangs the UI (see #3640)
  • colors and fonts are not honored in the exported report
  • at one point in testing the features, there was an alert box presented that reported a problem trying to "save column changes", it is not clear why this was shown but it seems like a bug: existing check on directory access rights is insufficient: we need to check for more rights.
  • when cells font is altered, font height (and therefore row height) may be incorrectly calculated for some fonts in web
  • laggy popup menu selection in classical swing theme
  • sometimes abend occurs when enhanced settings reverted to 4GL defaults in the main browse in a customer app screen

Created task branch 3261d from P2J trunk revision 11276.

1. Depending on how fast the sorting works, we can apply changes in sorting immediately or draw Apply Sorting / Undo Sorting / Reset Sorting buttons when user starts altering sorting criteria.
2. Method 1 of altering sorting criteria: clicking on a column header, changes sorting direction in ascending - descending - no sorting order. Also it becomes the most coarse sorting criterion. If there are multiple sorting columns, sort number is drawn, where "1" is the most coarse sorting.
3. Method 2 is to add column-specific options "Sort Ascending", "Sort Descending" and "Reset Sorting" to the right-click menu. We can combine these two methods.

I assume we want to implement multiple-column sorting criteria.

Yes.

We have sorting arrows and sorting numbers as indicators.

Yes, this makes sense.

Method 1 of altering sorting criteria: clicking on a column header, changes sorting direction in ascending - descending - no sorting order.

I prefer this.

Also it becomes the most coarse sorting criterion. If there are multiple sorting columns, sort number is drawn, where "1" is the most coarse sorting.

Good. I assume that subsequent columns clicked become the 2, 3...?

draw Apply Sorting / Undo Sorting / Reset Sorting buttons when user starts altering sorting criteria

This seems safest since we can't know how long the re-sort will take.

I don't think we need "Undo Sorting". Let's keep this simple, only provide Apply Sorting and Reset Sorting. Reset should clear all user-provided sorting and take it back to the original programming from the 4GL.

Where do you plan to put these buttons? I'm worried that if they are in the popup menu, the user will not know where to find them (it is not obvious enough). I think they need to be visible directly as soon as the user starts editing the sorting criteria.

Good. I assume that subsequent columns clicked become the 2, 3...?

Yes.

Where do you plan to put these buttons?

I'm planning to put then on a small panel over cells data, bound to the left edge, right under labels.

Where do you plan to put these buttons?

I'm planning to put then on a small panel over cells data, bound to the left edge, right under labels.

How about centering it horizontally in the browse (still right under the labels)? The idea: on average the user will have to move the mouse less to click one of these buttons than if it was all the way to the left.

From Ovidiu via email w.r.t. the persistence support for filtering:

While testing I found some cases when the HQL/SQL queries end with errors. These cases are related to optimised queries for server-side, when the filtered predicate is used in the optimised query but when the filter is removed the optimised SQL query string is not re-evaluated, although the arguments are. As result, HQL Preprocessor work will fail with ArrayIndexOutOfBoundsException.

I am working on forcing the server joins to be reevaluated each time the query filters change.

In CompoundQuery, can you reset components to null, work with originalComponents instead, and let it re-optimize the new query? It's probably better to let the Optimizer make its decision based on the changes anyway. I hadn't thought of this before.

Eric Faulhaber wrote:

In CompoundQuery, can you reset components to null, work with originalComponents instead, and let it re-optimize the new query? It's probably better to let the Optimizer make its decision based on the changes anyway. I hadn't thought of this before.

This seems like a better idea. My initial solution to alter directly the already optimized was incorrect. I started working with the originalComponents instead, as suggested, but momentarily the filters are not working at all. I am investigating this. Probably I am too aggressive. Yet, the optimizer tries to do its job, but I don't see the dynamically added terms in the final processed query.

Thanks to Eric's idea the dynamic filtering is now running for compound queries.
While testing the new filtering parches I found and fixed particular cases of sorting.

Committed to 3261d as r11278.

Code review 3261d/11278.

None of the changed files have history entries.

QueryComponent.resolveArg was made private; please move it accordingly.

I think the changes are ok, but frankly, there is too much complexity in this feature set for me to fully understand the impact with just a code review. How are you testing these?

Stanislav, have you been following this set of changes to be sure you are on the same page with Ovidiu's changes to support sorting and filtering?

Stanislav, have you been following this set of changes to be sure you are on the same page with Ovidiu's changes to support sorting and filtering?

Eric, I've started with conversion and then visual aspects so I didn't get a chance to connect with Ovidiu's changes yet.

When can you have a working version that can be tested/shown to a customer?

When can you have a working version that can be tested/shown to a customer?

Around the next Monday.

Rebased task branch 3261d from P2J trunk revision 11278.

Ovidiu, I'll be posting dynamic sorting/filtering issues I'll find. I'll try to find maximum of them before you go to vacation.

1. login window in customer app doesn't work: "Entry 0 is outside the range of the list (560)".

Stanislav Lomany wrote:

Ovidiu, I'll be posting dynamic sorting/filtering issues I'll find. I'll try to find maximum of them before you go to vacation.
1. login window in customer app doesn't work: "Entry 0 is outside the range of the list (560)".

Have you sorted out this range issue? I don't recall encountering it and I have no idea where it comes from. Do you have a call-stack for it?

Have you sorted out this range issue? I don't recall encountering it and I have no idea where it comes from. Do you have a call-stack for it?

It is about the changes in RandomAccessQuery.java and QueryComponent.java. I guess something is messed in args/dfArgs. No stack trace because, I guess, some entity silently is not found in the code preceding the error message. Could you take a look?

Stanislav Lomany wrote:

It is about the changes in RandomAccessQuery.java and QueryComponent.java. I guess something is messed in args/dfArgs. No stack trace because, I guess, some entity silently is not found in the code preceding the error message. Could you take a look?

I would like to, but I don't know where to too. This is why I asked for a stacktrace, to see who/where genOutsideRangeError() is called. In fact, it is called only from Text.replaceEntry() and TextOps.entryImpl(). I could not establish a direct connection of these two and my changes related to dynamic filter/sorting of browsed queries.
Please publish the testcase that generates this error (560).

There is no testcase, I just can't login the customer app. The data on on the login screen is not filled with this error.

I'm debugging it.

Stanislav Lomany wrote:

There is no testcase, I just can't login the customer app. The data on on the login screen is not filled with this error.

Indeed, there is a regression there. I found some uninitialized data in RandomAccessQuery. I am finding now a solution.

Stanislav,
Please update to revision 11282. After the variables were initialized the client application started working properly for me.

One more feature I would really like to get into the enhanced browse is the ability to "stripe" the rows (alternating lighter and darker background row colors, with grid lines drawn only vertically). The background colors should be configurable. What would be the estimated effort to implement this?

One more feature I would really like to get into the enhanced browse is the ability to "stripe" the rows (alternating lighter and darker background row colors, with grid lines drawn only vertically). The background colors should be configurable. What would be the estimated effort to implement this?

• flag to disable drawing row separators
• flag to enable using different background colors for alternating lines

1. I suggest to mark non-sortable columns with the header colored with light red in "sort adjustment mode" (when "Apply"/"Revert" buttons are shown).
2. Do we want to keep the line of columns filters always visible? Maybe we can make them 1. collapsible and 2. hide by default
3. Are you OK with the following logic: when user updates a filtering field, he can press Enter to apply changes. If he clicks outside the entry field, "Apply"/"Revert" buttons are show (applied/reverted for the given column only).

I like all 3 ideas. Go ahead with them.

FYI in order to save time and avoid future conflicts with the gear icon in the top-left corner, I'll make enabling/disabling on the filtering entries thru the enhanced menu so far.

OK.

1. the main problem: filter values are applied using KA_RETURN. But in the customer's app KA_RETURN triggers the frame's DEFAULT-BUTTON instead. The fastest and reliable solution I see is to display Apply/Revert filtering buttons when user starts changing a filter value and apply the new value(s) only on clicking "Apply". Thoughts?
2. No "clear filter value" cross-like buttons.
3. Unknown filter values are not empty strings.
4. Invalid position of sorting arrows if filtering fields are shown.

the main problem: filter values are applied using KA_RETURN. But in the customer's app KA_RETURN triggers the frame's DEFAULT-BUTTON instead. The fastest and reliable solution I see is to display Apply/Revert filtering buttons when user starts changing a filter value and apply the new value(s) only on clicking "Apply". Thoughts?

Can you do the equivalent of registering a RETURN trigger for each of the filter-specific FILL-IN widgets?

This would naturally process the event and the frame-level processing would never occur.

Or as an alternative, implement a subclass of FILL-IN that completely disables trigger processing on these widgets and instead implements just the features needed. There is no reason to expose the 4GL behavior in these filter fields.

Unknown filter values are not empty strings.

Can you explain what you mean by this?

Unknown filter values are not empty strings.

This feature: "I suppose that for non-filtered columns fill-ins should be empty. Note that "?" is displayed for unknown numbers in fill-ins, we should avoid that." Never mind, already implemented.

Status of filtering work from Stanislav:

1. Apply/Revert window. So far you can apply filters by pressing Return in a filtering field. All filters can be reset by hiding the filtering panel.
2. "Reset to unknown" cross-like buttons. So far you can reset a value by typing "?" in the field.
3. Invalid positioning of sorting arrows if filters are shown.
4. Write javadocs / rebase.

Using filters with editable browse can cause drawing bugs. Also there are some random bugs here and there.

Is sorting complete (except for the limitations noted in #3261-421?

Is sorting complete (except for the limitations noted in #3261-421?

Yes.

Since items 1 and 2 have workarounds, it seems like the following should be done first:

3. Invalid positioning of sorting arrows if filters are shown.
4. Write javadocs / rebase.
Using filters with editable browse can cause drawing bugs.

After that work on items 1 and 2.

In order to enable enhanced sorting and filtering, use these parameters in the directory (the first one is required because enhanced filtering so far is enabled thru the right-click menu):

<node class="boolean" name="enhancedBrowseEnabled">
   <node-attribute name="value" value="TRUE"/>
</node>
<node class="boolean" name="enhancedBrowseSortingEnabled">
   <node-attribute name="value" value="TRUE"/>
</node>
<node class="boolean" name="enhancedBrowseFilteringEnabled">
   <node-attribute name="value" value="TRUE"/>
</node>

Greg Shah wrote:

Status of filtering work from Stanislav:

1. Apply/Revert window. So far you can apply filters by pressing Return in a filtering field. All filters can be reset by hiding the filtering panel.
2. "Reset to unknown" cross-like buttons. So far you can reset a value by typing "?" in the field.

What does "reset to unknown" mean? Turning off filtering and resetting to the original query state, or something else? I was unable to type "?" into any filter field (it just would not display). Removing any entry and hitting <Enter> seemed to work, though.

3. Invalid positioning of sorting arrows if filters are shown.

By this, do you mean the red column header highlighting? What is that? I noticed this on the headers of some columns unrelated to the column I chose for sorting, but the arrow seemed ok. I only tested one particular browse in a customer app, so maybe I didn't see what you are referring to here.

Does the red highlight mean this column is not sortable? If so, how is that determined?

4. Write javadocs / rebase.

Using filters with editable browse can cause drawing bugs. Also there are some random bugs here and there.

The bugs I encountered:

• Filtering on a numeric column did not display the one row which had the value on which I was filtering. I don't know if it is the numeric type which caused this, but filtering did seem to work on a column containing non-numeric text.
• I was unable to type more than 8 characters in a particular column's filter field. Perhaps a format string is preventing this?
• In some cases of filtering, I got a loading record error (91). I have not yet figured out the exact recreate.

In terms of priorities, I agree with Greg's response in #3261-424.

What does "reset to unknown" mean? Turning off filtering and resetting to the original query state, or something else?

Please take a look at the picture in #3261-319. This button resets filtering for a particular column.

I was unable to type "?" into any filter field (it just would not display). Removing any entry and hitting <Enter> seemed to work, though.

Unknown values are rendered as empty strings, so everything worked as expected. This is not obvious though, so that's why we need the button mentioned above.

3. Invalid positioning of sorting arrows if filters are shown.

By this, do you mean the red column header highlighting? What is that? I noticed this on the headers of some columns unrelated to the column I chose for sorting, but the arrow seemed ok. I only tested one particular browse in a customer app, so maybe I didn't see what you are referring to here.

You can display the filtering row and click a header to trigger enhanced sorting. The sorting arrow is rendered under the filter field.

Does the red highlight mean this column is not sortable? If so, how is that determined?

Yes. It can be explicitly defined as non-sortable or its content is not filled using an underlying field reference (e.g. it is computed).

• Filtering on a numeric column did not display the one row which had the value on which I was filtering. I don't know if it is the numeric type which caused this, but filtering did seem to work on a column containing non-numeric text.

I just found that filtering doesn't seem to work for decimals. I'll look into it.

• I was unable to type more than 8 characters in a particular column's filter field. Perhaps a format string is preventing this?

I apply column's format to the filtering field. Let me know if some column doesn't match this rule or you do not find this rule appropriate.

• In some cases of filtering, I got a loading record error (91). I have not yet figured out the exact recreate.

I haven't seen this particular error.

Do you expect this branch will be difficult to get into the trunk or should regressions primarily be only visible when enhanced mode is enabled?

Do you expect this branch will be difficult to get into the trunk or should regressions primarily be only visible when enhanced mode is enabled?

The last one.

I just found that filtering doesn't seem to work for decimals. I'll look into it.

I've fixed the issue, it wasn't datatype-related.

Rebased task branch 3261d from P2J trunk revision 11281.

I've merged 3261d into 3601a as rev 11310. However after the merge there is an issue: line of filters may not be repainted on show/hide. There are messages of this kind in the client log:

WARNING: Wasted event: com.goldencode.p2j.ui.client.event.PaintEvent

Regarding the MAJIC regression, there is an extra zero record in the browse.

UX question. Consider a browse has both enhanced sorting and filtering enabled. How should it behave if user tries to alter both sorting or filtering simultaneously - should "apply sorting AND filtering" window be displayed in this case or simultaneous editing of sorting and filtering should be prohibited? As the most conflicting case consider that user started modifying a filter entry and then clicked a header to adjust sorting.

Sorting and filtering should be allowed to be combined. This would certainly be the expectation of the user.

should "apply sorting AND filtering" window be displayed in this case

Yes.

I think the current approach for storing/reading/saving layout and UI settings cannot be used in a production environment. Reasons:

• The large number of locations in the directory in which the data can be stored results in the requirement for very complex permissions to be created and maintained. These are error prone and cannot be reasonably maintained in the field. For example, it is not reasonable to assume that an admin will be able to get this right. This is especially a problem for account level details.
• Although some customers do directly leverage the FWD security model (where each user will have a FWD account associated with their usage), the most common approach is to use the Do It Yourself (DIY) 4GL approach where the security is largely written in the application itself. In this approach, there is often some use of the USERID (check who is the current user), SETUSERID (set the current user) and _user table (to check the @ENCODE@d password against the stored value). Such an installation will have the users all share the "bogus" FWD account making it impossible to use any account level permissions or account level saving of state.
• The menu items for saving change are too complicated because they "explode" the possible levels of storage (global, server, group, user) by the (per browse or all browses) and by multiple groups. Since this is done as separate menu items (see this screen shot for an example), it is hard to see how a user would have any idea of what to choose. They would have to have real understanding of how the FWD security model and directory work. That is not practical. Even an admin won't understand that and won't have the time to learn it just for this feature.

I think we still need to store this state in the directory. Using an file system level properties (or other config) is nasty and not suitable for web and cloud based environments. Putting it in the database is nasty because it is adding something managed by FWD to an application-level resource.

The approach I am suggesting:

• Do not map to the global/server/group/user lookup processing.
• The following lookup levels would be possible:
  • Global Default (affects all users and all browses)
  • Browse-Specific Default (affects a specific browse but for all users)
  • User Default (affects all browses for a specific user)
  • Browse-Specific User Settings (affects a specific browse for a specific user)
• Only the admin should be allowed to save state that affects multuple users (Global Default or Browse-Specific Default).
• Store all state under a single location in the directory, which is specific to enhanced browse.
• The USERID as reported to the 4GL would be used to create any user-specific section inside the enhanced browse area.
• The USERID as reported to the 4GL would be used to know if a user is an "admin". This diverges from the FWD security model, but it seems consistent with a reasonable approach to the 4GL.
• A single ACL could provide permissions to edit the enhanced browse section of the directory. This would by default be set for bogus and be shared by all users.
• To reduce the security exposure of opening up directory editing to all users:
  • The actual directory editing code will need to be on the server side. It will need to ensure that only the enhanced browse layout/UI settings can be changed based on user input. It will need to determine the difference between the admin and non-admin users to know if the all-users portions can be changed and the menus should be created accordingly.
  • The client side code must be restricted to up-calling to the server with the settings and the level to which it is being saved. Compromise of the client code and/or the parameters in the up-call should not allow the user to make unauthorized edits.
• Any setting that only makes sense for a specific browse (e.g. column widths) would not be saved as an all-browse default.

I think this approach is workable. Does anyone have any questions, comments or concerns?

Stanislav: Do you think this can be put together quickly? It seems much easier and simpler than the current implementation and I think everything that is needed here is already well known or part of the existing approach. How much time is needed for this?

I think this approach is workable. Does anyone have any questions, comments or concerns?

I like it. I don't like complicated access rights we have now.

Stanislav: Do you think this can be put together quickly? It seems much easier and simpler than the current implementation and I think everything that is needed here is already well known or part of the existing approach. How much time is needed for this?

A day I suppose. Let me know about priority.

I've added "rework of the Save Changes processing and directory approach for layout/UI customization as described in #3261-436" to #3261-390.

For priorities, I've added this to #3706-20.

Are there any features described in this task which are not complete AND not documented in #3880? I want to make sure #3880 has a full list of all remaining work before I close this task.

Are there any features described in this task which are not complete AND not documented in #3880?

I don't see any.