FWD » Core Development » User Interface

I came up with a design that limits the amount of changes to current ChUI dialog implementation and GUI widget structure with respect to the relatively short time frame.

The idea is to create a new class FrameWindowGuiImpl that will host a FrameGuiImpl instance, will manage its lifetime, route necessary events, etc. The changes to the current structures and logic should be minimal, most importantly the places where FrameConfig.dialog is handled will have to be modified to account for the new GUI use cases.

The approach seems reasonable.

Created task branch 2560a from trunk revision 10937.

Rebased task branch 2560a from P2J trunk revision 10940. The current revision in 2560a is now 10945.

Please provide a detailed list of the remaining work/testing that needs to be done before you believe this is ready for code review. I'd also like an estimate of the time needed for those items.

• Fix multiple show/hide of the same frame/dialog-box. 0.25 MD
• Fix default dialog-box position. 0.25 MD
• More tests and fixing potential differences of event handling in the dialog-box. I've found already some some minor differences. So far I can say this will take 1.5 MD.
• Majic regression testing. This is already in progress with passing Ctrl+C. 0.5 MD

Hynek Cihlar wrote:

Left known issues and todo tasks are:

• Fix multiple show/hide of the same frame/dialog-box. 0.25 MD
• Fix default dialog-box position. 0.25 MD
• More tests and fixing potential differences of event handling in the dialog-box. I've found already some some minor differences. So far I can say this will take 1.5 MD.
• Majic regression testing. This is already in progress with passing Ctrl+C. 0.5 MD

I forgot that #1811 has some conflicting changes, this will need to be merged and retested. I expect 1 MD for this.

I forgot that #1811 has some conflicting changes, this will need to be merged and retested. I expect 1 MD for this.

Are you wanting 1811r to merge to trunk before 2560a?

Greg Shah wrote:

I forgot that #1811 has some conflicting changes, this will need to be merged and retested. I expect 1 MD for this.

Are you wanting 1811r to merge to trunk before 2560a?

Yes, let's merge 1811r first. It is a bigger chunk of changes than 2560a.

Rebased task branch 2560a from P2J trunk revision 10945. The current revision in 2560a is now 10967.

Rebased task branch 2560a from P2J trunk revision 10947. The current revision in 2560a is now 10974.

Code Review Task Branch 2560a Revision 10976

Generally, I don't have many issues with this update.

1. Our window support is now more complicated than in the past. I don't think it is clear to readers of the code how and why the hierarchy is designed the way it is. There is frequent usage of instanceof to differentiate between various levels in the hierarchy, where normally there would be overridden methods that just implement differently. The purpose behind TopLevelWindow, TitledWindow, Window and so forth is not easily understandable. I don't think it is written down anywhere and I'm getting confused myself. I think it also leads to a very fragile implementation which is why it is constantly regressing and breaking as we extend the system. Since we are "under the gun" to get things done, we need to schedule some time (probably after the next 2 weeks) for you to write up some documentation (hopefully in javadoc) to summarize the core ideas behind the approach. Hopefully that will be reduced after 2560a is merged to trunk, but I think the documentation will be quite important to help us deal with the fragility in the time before we can find a better/less fragile implementation.

2. The FocusManager.getFirstEnabledWidget() TODO about dialog windows seems pretty important. What is the implementation plan?

3. What is the reason for adding import org.apache.commons.lang.*; to WindowManager, FrameGuiImpl?

4. FrameGuiImpl.location() and FrameGuiImpl.physicalLocation() are missing javadoc.

5. ModalWindow.isRealized() and ModalWindow.realize() are missing javadoc.

6. GuiWindowCommons needs class-level javadoc.

Greg Shah wrote:

Code Review Task Branch 2560a Revision 10976

Generally, I don't have many issues with this update.

1. Our window support is now more complicated than in the past. I don't think it is clear to readers of the code how and why the hierarchy is designed the way it is. There is frequent usage of instanceof to differentiate between various levels in the hierarchy, where normally there would be overridden methods that just implement differently.

Yes, agree, but please note that my changes effectively add 4 instanceof's and remove 2. The others are changes of methods moved to more appropriate places in the window class hierarchy and WindowManager. I will document those new instaceof's to clarify, why they were needed.

The purpose behind TopLevelWindow, TitledWindow, Window and so forth is not easily understandable. I don't think it is written down anywhere and I'm getting confused myself. I think it also leads to a very fragile implementation which is why it is constantly regressing and breaking as we extend the system.

I agree.

2. The FocusManager.getFirstEnabledWidget() TODO about dialog windows seems pretty important. What is the implementation plan?

Good point. I need to find the right use case and then decide how to deal with the TODO. I wouldn't want to delay the intergation of 2560a because of this though.

3. What is the reason for adding import org.apache.commons.lang.*; to WindowManager, FrameGuiImpl?

In WindowManager the reason was the reference of ArrayUtils.toPrimitive but I think that can be replaced with the inhouse Utils class.

In FrameGuiImpl the reason is the use of ObjectUtils.equals. I will replace this with Objects.equals.

I need to find the right use case and then decide how to deal with the TODO. I wouldn't want to delay the intergation of 2560a because of this though.

This is reasonable. You can put that into 1811s which is where we are collecting many GUI fixes.

Checked-in 2560a revision 10980:

• addressed the points from review,
• fixed all failed chui tests from the last Majic regression run,
• other unrelated GUI fixes and improvements.

Note that there are some missing file header entries. I will address this tomorrow. Regression test in progress.

One more chui regression from the last Majic test run, being fixed now.

Code Review Task Branch 2560a Revision 10980

I'm good with the changes.

Code Review Task Branch 2560a Revision 10984

I'm fine with the changes. I assume you'll javadoc and header them before you are done.

In regard to the tracing utility, we have wanted to add tracing for a while. This version is a bit too specific to UI, but it is fine for now. I think we will extend/improve it later. One thought: make Trace only contain very generic stuff (everything in it now except for window, x1, x2, y1, y2) and allow the user to sub-class it to add more specific data. Also Trace should be called something like TracePoint or TraceEvent to make it clear that it represents a captured data point.

2560a revision 10986 fixes the found ChUI regression. The problem was caused by a latent bug in browse's focus processing. Majic regression testing restarted.

I didn't originally plan to keep it but if you think it may be useful I will put it to a more appropriate package, maybe com.goldencode.p2j.

Please use com.goldencode.trace instead.

The regression results of 2560a revision 10986 show two suspicious failed tests, gso_245 and tc_codes_crew_003. When tested manually they pass. I am rerunning the main part to confirm these are negative failures.

Please post the final, clean code to bzr.

Greg Shah wrote:

Please post the final, clean code to bzr.

Checked in, task branch 2560a revision 10987.

Code Review Task Branch 2560a Revision 10987

I'm good with the changes.

What is the status of testing?

Greg Shah wrote:

Code Review Task Branch 2560a Revision 10987

I'm good with the changes.

What is the status of testing?

gso_245 and tc_codes_crew_003 passed, I think we can consider the branch stable enough.

Have you re-run any of the GUI testcases to see if there are regressions?

What is the status of #2702, #2752, #2729, #2564 note 90 and the move window test?

Greg Shah wrote:

Have you re-run any of the GUI testcases to see if there are regressions?

I have run the dialog-box specific testcases, some browse, combo box and selection test cases. I din't run all the GUI test cases though.

What is the status of #2702, #2752, #2729, #2564 note 90 and the move window test?

There are frame layout issues which affect tiny input as well as dialog-box. There have been changes in #1811/1811s which may resolve these. When 1811s is released these will have to be re-checked.

#2702: Tiny input tested and works with the limitation above.

I didn't check #2752/#2704, there didn't seem to be any connection with this redmine issue.

#2729: alert-box retested and works. I now retested editorWidget.p and the alert-box part works ok.

#2564: I won't be able to retest this before the related p2j changes are merged to trunk. I wasn't able to convert the test files.

Also please note that I will need to do more testing on the production source code.

As far as you know, does this update cause any new regressions to GUI?

Greg Shah wrote:

As far as you know, does this update cause any new regressions to GUI?

I don't know of any new regressions.

OK, please merge to trunk now.

In your commit message, make sure to also reference #2702 and #2729 (e.g. using Refs #2560, #2702, #2729.).

#2560a checked in to trunk as revision 10948.

There are frame layout issues which affect tiny input as well as dialog-box. There have been changes in #1811/1811s which may resolve these. When 1811s is released these will have to be re-checked.

Other than this, are there any other open items for this task?

Greg Shah wrote:

There are frame layout issues which affect tiny input as well as dialog-box. There have been changes in #1811/1811s which may resolve these. When 1811s is released these will have to be re-checked.

Other than this, are there any other open items for this task?

Yes, the production code retest.

Hynek Cihlar wrote:

Greg Shah wrote:

There are frame layout issues which affect tiny input as well as dialog-box. There have been changes in #1811/1811s which may resolve these. When 1811s is released these will have to be re-checked.

Other than this, are there any other open items for this task?

Yes, the production code retest.

Plus the TODO: dialog-box left in the source code.

Hynek Cihlar wrote:

Hynek Cihlar wrote:

Greg Shah wrote:

There are frame layout issues which affect tiny input as well as dialog-box. There have been changes in #1811/1811s which may resolve these. When 1811s is released these will have to be re-checked.

Other than this, are there any other open items for this task?

Yes, the production code retest.

Plus the TODO: dialog-box left in the source code.

Any additions or changes will be done in 1811s (or if they are ready after we need to go into testing for 1811s, then in some other future branch).

Ok, I will check the known issues after 1811s is rebased.