FWD » Core Development » User Interface

Is this the next task in queue, correct?

Actually, I'm trying to avoid doing this task for the current project. I am waiting to hear back from the customer. Please hold off on this task for now.

Greg Shah wrote:

Actually, I'm trying to avoid doing this task for the current project. I am waiting to hear back from the customer. Please hold off on this task for now.

OK.

Implement everything from scratch keeping look and feel to match the Windows style. In this model all elements are drawing and working using Java tools.

For future reference, we will have to implement this approach. Everything must work in a cross-platform manner. I realize this means we have to duplicate the look and feel of the Windows dialog.

Please note that in regard to the web client, it is not clear if we need to enable inspection/selection of files at the browser-side. Most likely the correct solution is to have this dialog show the files from the Java client system, even if the UI is being rendered in the browser. This is consistent with how we do all other file system access, so I think that is the correct solution.

I agree, the approach is reasonable.

Code Review Task Branch 1830a Revision 11164

I really like the chaining implementation. Its design is smart and your implementation in uistatements is well written (reusable code) and easy to understand.

1. ColorTable.ColorChooserDialog needs javadoc.

2. ChuiWidgetFactory, GuiWidgetFactory, WidgetFactory, WidgetFactoryAdapter are missing their history entries.

3. The ThinClient.pause() changes to add wnd.window().setVisible(true); seems dangerous. Doing this will realize the window, which has many implications. Matching window realization exactly with the same locations in the 4GL is thus quite important. How safe do you think this change is?

4. Please move the UI dialog processing out of the FileSystemOps class. Such processing (even though it is file system related), should be exclusively in the ui/client/ package.

5. Please move HelpService and PrintingService out of util and into ui.

6. FileDialogGuiImpl should be in ui/client/gui/.

7. FileDialogImpl should be in ui/client/chui/.

8. AbstractFileChooserDialog, DirDialogImpl, FileDialogGuiImpl need standard headers to be added.

Greg Shah wrote:

Code Review Task Branch 1830a Revision 11164

I really like the chaining implementation. Its design is smart and your implementation in uistatements is well written (reusable code) and easy to understand.

Thank you. Reusability was one of the goals when I implemented this. I wanted to push this mechanism up (to AAST/CommonAstSupport) so that other statements with lots of options can benefit from it, but changes are major. I decided to let is as a POC for now and implement this in TRPL for SYSTEM-DIALOGs only.

1. ColorTable.ColorChooserDialog needs javadoc.

Added.

2. ChuiWidgetFactory, GuiWidgetFactory, WidgetFactory, WidgetFactoryAdapter are missing their history entries.

Added.

3. The ThinClient.pause() changes to add wnd.window().setVisible(true); seems dangerous. Doing this will realize the window, which has many implications. Matching window realization exactly with the same locations in the 4GL is thus quite important. How safe do you think this change is?

You are right about realization of windows. However, I noticed that at some moment, the ABL shows the window but FWD does not.

4. Please move the UI dialog processing out of the FileSystemOps class. Such processing (even though it is file system related), should be exclusively in the ui/client/ package.

I moved the two factory methods from FileSystemOps class to FileDialog. However, I think they can stay to FileSystemOps. The methods do not create true UI dialogs, instead FileDialog is a container that collect options (via method chaining), and then the request is sent to client based on those options. The client will create true dialogs for user interaction and send the result back to blocking FileDialog.execute() method.

5. Please move HelpService and PrintingService out of util and into ui.

Done.

6. FileDialogGuiImpl should be in ui/client/gui/.

Done.

7. FileDialogImpl should be in ui/client/chui/.

Done.

8. AbstractFileChooserDialog, DirDialogImpl, FileDialogGuiImpl need standard headers to be added.

Added. Javadocs too.

Committed revision 11165. This branch will soon be merged into 1818a.

I moved the two factory methods from FileSystemOps class to FileDialog. However, I think they can stay to FileSystemOps. The methods do not create true UI dialogs, instead FileDialog is a container that collect options (via method chaining), and then the request is sent to client based on those options. The client will create true dialogs for user interaction and send the result back to blocking FileDialog.execute() method.

I'm trying to move interactive (UI) code out of util. Over time we may do many things with the code in util, including running client portions on the server or in remote processes. When there are dependencies on an interactive UI, even if hidden in a secondary class like FileDialog), it makes it harder to do non-interactive things with the util code.

Some time ago we removed many of the UI references from util. I don't want to add new ones. Thanks for moving this out.

Code Review Task Branch 1830a Revision 11165

The changes are good.

Code Review Task Branch 1830b Revision 11199.

The changes are impressive, hands down. I found only some minor issues.

• build.xml is missing change history entry.

• It may be a bit confusing that ColorRgb class besides RGB also declares HSL fields and related HSL logic.

• TC.chooseFont() contains:
  

  // focus has to be restored no matter enabled or not
  // focusManager.setFocusOn(saveFocus);
  

  
  Should the focus be restored? If not, please remove the lines so that they don't confuse the reader.
  Same for the file/dir and color dialogs.

• Is posting of FOCUS_GAINED in TC.chooseFont needed? The font dialog is a system dialog which should not process legacy events, right? Same for the file/dir and color dialogs.

• In TC.chooseFont the call to FM.setFont is commented out and so the settings from the font dialog don't get applied anywhere. Is this expected?

• ComboBox.itemListeners is missing is missing javadoc.

• In ComboBox there is a comment: "TODO: this is needed to deliver changes to client-side. Not fully tested for side-effects." Is this something that could possibly break? If so it should be tested. If not, the TODO should go away.

• There are multiple debug outputs with System.(out|err).println, these should be removed.

• DirDialogImpl is missing file change history entry.

• FileDialogImpl.defaultExtension is not used anywhere, it only gets assigned in setFilters.

• FontChooserGuiImpl.Preview inherits from ButtonGuiImpl. Why not AbstractWidget? Is the Preview using any of the button features?

• Instead of
  background = loadImage(ThemeManager.getCurrentTheme().getName() + "/colordialog/gamut.png");
  we should probably have a loadImage method on the ThemeManger. The resource location should be an implementation detail of the theme manager.

• GamutPicker.loadImage should at least log the IOException.

• ContentThumbView.draw, ContentIcon and DetailsIcon are missing comments.

• Several methods in FileChooserUtils are missing comments.

• FileDialogGuiImpl.showHistory is not implemented, is this expected?

• ButtonsPane.setWidgetLocation is missing javadoc.

• ListThumbView.ListItem is missing javadoc.

• SimpleLabel.ctor contains ""// TODO ?"

• SmallIconsThumbView.SmallIcon is missing javadoc.

• TileIcon is missing javadoc.

Ovidiu, I added you as the issue watcher. Please see the review in note 18.

Hynek,

I got the notification because I am the assignee. Thank you very much for the quick review. I will do the fixes during the weekend.

Ovidiu Maxiniuc wrote:

Hynek,

I got the notification because I am the assignee. Thank you very much for the quick review. I will do the fixes during the weekend.

Btw. to give you heads up, the changes will conflict around the generic modal dialog changes with 1795b which I am about to check in (once the ChUI regression tests pass).

Hynek Cihlar wrote:

• build.xml is missing change history entry.

Added

• It may be a bit confusing that ColorRgb class besides RGB also declares HSL fields and related HSL logic.

I created a new class specialized in HSL color space. I added methods to convert from and to RGB.

• TC.chooseFont() contains:
  [...]
  Should the focus be restored? If not, please remove the lines so that they don't confuse the reader. Same for the file/dir and color dialogs.

Focus restored to previous holder.

• Is posting of FOCUS_GAINED in TC.chooseFont needed? The font dialog is a system dialog which should not process legacy events, right? Same for the file/dir and color dialogs.

These are system dialogs, but they are built with FWD client-side components. The focus should move naturally from last widget holding it to the first widget in the new dialog.

• In TC.chooseFont the call to FM.setFont is commented out and so the settings from the font dialog don't get applied anywhere. Is this expected?

The method did not exist. Added implementation of FontManager.setFont.

• ComboBox.itemListeners is missing is missing javadoc.

Fixed.

• In ComboBox there is a comment: "TODO: this is needed to deliver changes to client-side. Not fully tested for side-effects." Is this something that could possibly break? If so it should be tested. If not, the TODO should go away.

Not yet tested. I added the reminder because I am not aware of the consequences. Do you have any idea how can I pinpoint these, this beside standard devsrv01 and manual tests?

• There are multiple debug outputs with System.(out|err).println, these should be removed.

I removed then or replaced them with LOG.log statements.

• DirDialogImpl is missing file change history entry.

Added.

• FileDialogImpl.defaultExtension is not used anywhere, it only gets assigned in setFilters.

Fixed.

• FontChooserGuiImpl.Preview inherits from ButtonGuiImpl. Why not AbstractWidget? Is the Preview using any of the button features?

Switched to AbstractWidget.

• Instead of
  background = loadImage(ThemeManager.getCurrentTheme().getName() + "/colordialog/gamut.png");
  we should probably have a loadImage method on the ThemeManger. The resource location should be an implementation detail of the theme manager.

Indeed. I prefer to delay this for a future iteration.

• GamutPicker.loadImage should at least log the IOException.

Failed to load resource will be logged.

• ContentThumbView.draw, ContentIcon and DetailsIcon are missing comments.

Javadocs added.

• Several methods in FileChooserUtils are missing comments.

Javadocs added.

• FileDialogGuiImpl.showHistory is not implemented, is this expected?

Indeed. I delay this for a future iteration.

• ListThumbView.ListItem is missing javadoc.

Javadoc added.

• SimpleLabel.ctor contains ""// TODO ?"

Removed.

• SmallIconsThumbView.SmallIcon & TileIcon is missing javadoc.

Javadocs added.

Committed revision is 11200. I tried to rebase but we seem to overlap with IsModal and ModalBox which target for the same goal. I will do the merge tomorrow.

After replacing ModalBox with IsModal interface and fixing other regressions caused by rebase operation I committed to revno 11203.
Now the comboboxes cannot be selected by mouse, but this is a known issue.

Code review 1830b revision 11208.

Here is a few minor points.

Please remove "Invalidates HSL encoding." from ColorRgb.

In FontManager there are now two very similar methods getFont and getFontDetails. It seems that getFont is subset of getFontDetails.

In TC.chooseColor please make "TODO: all widgets (part of the specified window) having this color are immediately redrawn." more explicit so that it is clear whether the statement describes the FWD or 4GL behavior.

In AbstractFileChooserDialog setSelectedPath should move below the public methods. Also shouldn't getSelection be renamed to getSelectedPath so that it matches selectedPath and setSelectedPath?

The parameter windowId in FontChooserGuiImpl.createInstance is not referenced. I think ths should hold the window id specified in the corresponding 4GL statement. This applies to the other dialog classes, too.

Code review 1830b revision 11208, part two.

In DirDialogGuiImpl the copyright statement should only contain the current year.

DirChooserLayoutManager implements statement contains fully qualified interface id. Is this needed?

FolderTree.root and doLayout are missing javadoc.

Folder.draw is missing javadoc.

FileChooserUtils steels a LOG instance from FileDialogGuiImpl.

In FileDialogImpl "File/Dir does not exists:" -> "File/Dir does not exist:"

In FileDialogImpl LOG.log(Level.WARNING, "Search not yet implemented [" + searchPattern + "]"); should instead be logged with UnimplementedFeature class.

And thanks for the javadoc fixes.

Hynek Cihlar wrote:

Code review 1830b revision 11208.

Please remove "Invalidates HSL encoding." from ColorRgb.

Done.

In FontManager there are now two very similar methods getFont and getFontDetails. It seems that getFont is subset of getFontDetails.

I dropped my getFont and wired to improved getFontDetails.

In TC.chooseColor please make "TODO: all widgets (part of the specified window) having this color are immediately redrawn." more explicit so that it is clear whether the statement describes the FWD or 4GL behavior.

This is the goal (4GL behavior). For the moment, FWD will apply the new color to new widgets that are created (I think because we usually cache the color resolver).

In AbstractFileChooserDialog setSelectedPath should move below the public methods. Also shouldn't getSelection be renamed to getSelectedPath so that it matches selectedPath and setSelectedPath?

Thanks for spotting this. Replaced.

The parameter windowId in FontChooserGuiImpl.createInstance is not referenced. I think ths should hold the window id specified in the corresponding 4GL statement. This applies to the other dialog classes, too.

The windowId should be window the new dialogs are modal to. However, the new classes are DialogBox frames. The windowId of such objects points to their top-level window (a DialogBoxWindow instance), otherwise topLevelWindow() ceases working.

In DirDialogGuiImpl the copyright statement should only contain the current year.

Fixed.

DirChooserLayoutManager implements statement contains fully qualified interface id. Is this needed?

Fixed.

FolderTree.root, doLayout and Folder.draw are missing javadoc.

Added.

FileChooserUtils steels a LOG instance from FileDialogGuiImpl.

Done.

In FileDialogImpl "File/Dir does not exists:" -> "File/Dir does not exist:"

Done.

In FileDialogImpl LOG.log(Level.WARNING, "Search not yet implemented [" + searchPattern + "]"); should instead be logged with UnimplementedFeature class.

Done.

I committed the changes to revision 11209. This revision also contain a fix for Back/Forward navigation in file chooser. I also dropped hidden path in file choosers (as windows does, by default).

• gso_17, step 79: an extra line in gso_17.sum file;
• tc_pay_sfc_003, step 55: the 180 sec elapsed. I think the server was too busy.

Ovidiu Maxiniuc wrote:

The parameter windowId in FontChooserGuiImpl.createInstance is not referenced. I think ths should hold the window id specified in the corresponding 4GL statement. This applies to the other dialog classes, too.

The windowId should be window the new dialogs are modal to. However, the new classes are DialogBox frames. The windowId of such objects points to their top-level window (a DialogBoxWindow instance), otherwise topLevelWindow() ceases working.

The window of windowId should be passed to DialogBoxWindow.setOwner() before the modal window is realized/created.

Btw. my print dialog has the same issue.

Hynek Cihlar wrote:

The window of windowId should be passed to DialogBoxWindow.setOwner() before the modal window is realized/created.
Btw. my print dialog has the same issue.

I use FrameGuiImpl.initialize. As you can see, at lines 403-405 its DialogBoxWindow is created and realized at once.

Ovidiu Maxiniuc wrote:

Hynek Cihlar wrote:

The window of windowId should be passed to DialogBoxWindow.setOwner() before the modal window is realized/created.
Btw. my print dialog has the same issue.

I use FrameGuiImpl.initialize. As you can see, at lines 403-405 its DialogBoxWindow is created and realized at once.

Yes, FrameGuiImpl is the place where the change must happen. But I think we can defer this, given the low severity of this and the potential of breaking things.

Branch 1830b was rebased to latest trunk (r11186). Current revision is 11210.

I tried to understand the extra line from gso_17.sum but it doesn't make sense. The gso_17.txt (which is created in same test) does not contain any reference to employee 1234.

It may be just a rare timing issue.

Eugenie: what do you think?

The extra line is (14):
1234 14.0 6.1 0.0 20.2 -7.9 18.4 16.0 -5.4

It is possible that I encountered this issue before, but I cannot remember for sure. Also, I can duplicate it each time (without restoring the db). And in subsequent manual tests the gso_17.txt file does have employee 1234 listed.

So, indeed, this seems like a synchronization issue. The gso_17.txt is generated first and gso_17.sum right after it. If the server was busy (there were 3 testing instances running) it is possible that another thread/driver to add employee 1234 before the .sum was generated.

Ignore this failure.

Greg,

After doing some manual tests I identified a possible regression in 1830b/11210 that caused a NPE. Because of the lack of time I did not further investigate this. Instead I rolled-back changes in ComboBox and patched it in new code so it does not require retesting. Committed revision 11212.

I am ready to merge this branch into trunk.

Please rebase and then merge to trunk.

Branch 1830b was rebased and merged to trunk as r11188.
The branch was archived and a notification email sent to team members.

An update from Ovidiu:

File dialogs are well connected with Windows Explorer. One can use it as a 'shell' and: create files/folders, rename, move them and even launch applications. Of course we will not support them all. I am not a fan of Windows explorer, however, the dialog continued to surprise me, these days. It saves user searches, allow user to filter files from crt directory by date/size/type. The images/text/pdf files have previews. Directories and folders have different icons depending on their content. Search is sometime fails to select a file from crt dir. The basic functionality is there but I could work this task the next whole year. The percent for this task depends on how deep we choose to go.

We do not need to go all the way. Can you please describe the list of current functionality and add some screen shots here? If the core is working well we will probably close this. The intention is to provide the basic function. But I need to know the specific list of things done and left to do before we make that decision.

These seem important:

• keyboard navigation
• search combobox
• New folder
• More options (at least the user should be able to pick the view mode)

Unless you disagree, these seem like ones we can defer:

• recent locations popup
• Organize button/popup
• Help button
• Hide folders

I think the buttons/menus should be removed which link to the missing parts otherwise the user will be confused.

What do you think?

Greg Shah wrote:

These seem important:

• keyboard navigation
• search combobox
• New folder
• More options (at least the user should be able to pick the view mode)

Unless you disagree, these seem like ones we can defer:

• recent locations popup
• Organize button/popup
• Help button
• Hide folders

I think the buttons/menus should be removed which link to the missing parts otherwise the user will be confused.

What do you think?

I agree. I will hide/disabled the non-working components.

Code Review Task Branch 1830c Revision 11229

Overall, the changes are really good. I especially like the better theme support for these dialogs. I have very little to suggest, though I must admit the changes are so large that I did skim some of them.

1. ContentThumbView.drawForeground() and (to a lesser degree) DetailsThumbView.drawForeground() are repetitively calling hasFocus(), isSelected() and isMouseOver(). Presumably, these could be called once and cached locally for the duration of the method.

2. The DirDialogGuiImpl constructor needs javadoc.

Hynek: please do a code review of 1830c.

I committed changes suggested in note 41 to revision 11230.

Regarding the LineEditor. There are a few changes related to mouse/focus handling in Windows that differ a bit from P4GL. I will do a in-depth check when Sergey finishes his changes.

Code Review Task Branch 1830c Revision 11230

TC.modalEventLoopWorker
The condition evt instanceof KeyInput && modalActive == 0 is never true since modalActive is always > 0 in the context of the method call.

Why the change of ComboBox.itemListeners from list to set? With the set the listeners will be invoked in indeterminate order, which (at best) could be source of confusion.

Please add the information to TC.isModalBoxActive() that the frame dialog-boxes do not count in modalActive even though they are technically modal windows (boxes).

ColorChooserGuiImpl.java
int max = rgb ? 255 : 240; // in RGB color space max is 256, in HSL is only 240
255 != 256

+      if (val < 0)
+      {
+         val = 0;
+      }
+      int max = rgb ? 255 : 240; // in RGB color space max is 256, in HSL is only 240
+      if (val > max)
+      {
+         val = max;
+      }

+      int max;
       if (val < 0)
+      {
+         val = 0;
+      }
+      // in RGB color space max is 256, in HSL is only 240
+      else if (val > (max = rgb ? 255 : 240))
+      {
+         val = max;
+      }

In ContentThumbView and DetailsThumbView there is a commented line // cfg.widthPixels = 400;

// setHeight(cc.heightFromNative(yOffset⁰)); in FolderTree.

EmptyThumbView.findRight()
int rx = 100000; => int rx = Integer.MAX_VALUE;
Better not to introduce any artificial limits. Similar in findLeft(), findUp() and findDown().

Use java.util.logging.Logger instead of Throwable.printStackTrace(); at multiple places.

FileDialogGuiImpl.createNewFolder in the catch handlers also log the exception object, otherwise the cause may be lost.

FileChooserUtils.isVirtualFolder(): '$' is a permissible character in the unix file systems, shouldn't this be somehow accounted for? Also in FileDialogGuiImpl.openFolder()@.

FileDialogGuiImpl.expandFolder() there is a block of commented out code.

FileDialogGuiImpl.expandFolder() (and createNewFolder(), navigate()) exception handler doesn't log the exception object.

FileDialogGuiImpl.onSelect() compares file system names as raw strings. Instead File or Path should be used.

FileDialogGuiImpl instead of "classic".equals(ThemeManager.getCurrentTheme().getName()) safer to compare against the theme class. Or maybe introduce special-purpose methods in ThemeManager for built-in themes (i.e. isClassic(), etc.).

Folder.lookup() is not used.

FileDialogGuiImpl.createNewFolder() will silently fail when there will exist directories "New Folder (n)" for all n>= 1 && n <= 999. Is this how Windows behave?

Folder.toString() use File.separator instead of "\".

FileDielogGuiImpl.SearchOperation
The way the search thread works can cause virtually unlimited number of threads to be spawned and eventually cause the client process to crash. The think is that each search selection event will start new thread, which itself checks whether another thread is not running only after it performs the file IO. Consider a slow network file system for example.

FileDialogGuiImpl, some of the fields contain "Note: Not implemented yet", but to me they seem to be implemented, like search.

Seeing the amount of new translatable strings I am wondering whether we should start thinking about i18n.

Seeing the amount of new translatable strings I am wondering whether we should start thinking about i18n.

Yes, we will need to do this sooner or later. The hard coded strings for error/warning messages and UI elements really should come from resource bundles.

Please make sure to update the gap analysis marking rules (e.g. rules/gaps/lang_stmts.rules) to update the status of all the SYSTEM-DIALOG-* variants. Right now the runtime support is marked as "stubs".

Greg Shah wrote:

Please make sure to update the gap analysis marking rules (e.g. rules/gaps/lang_stmts.rules) to update the status of all the SYSTEM-DIALOG-* variants. Right now the runtime support is marked as "stubs".

Thank you for reminding me. I completely forgot about gap maintenance.

A new update was committed to 1830c. The latest revision is 11258 after rebase.
Starting standard tests.

1830c/r11259 passed the AW and ChUI tests.

Code Review Task Branch 1830c Revision 11259

I'm good with the changes. I think this should be merged to trunk, but please coordinate with Eric because he is trying to get 3261b into trunk with high priority.

Branch 1830c was committed to trunk as r11240.
Notification email was sent to team members and branch was archived.