Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GUI dae: make more robust [Timebox 1.5 days] #8568

Open
Tom-Willemsen opened this issue Nov 13, 2024 · 4 comments · May be fixed by ISISComputingGroup/ibex_gui#1755
Open

GUI dae: make more robust [Timebox 1.5 days] #8568

Tom-Willemsen opened this issue Nov 13, 2024 · 4 comments · May be fixed by ISISComputingGroup/ibex_gui#1755
Assignees
@Chsudeepta
Labels
5 for release Required for next release no_release_notes Tickets that do not need release notes, use sparingly! review

Comments

@Tom-Willemsen
Copy link
Contributor

Tom-Willemsen commented Nov 13, 2024 •
edited by davidkeymer
Loading

Issue Description

As a user on OSIRIS, I would like to be able to change my DAE parameters (e.g. timing source) through the IBEX gui.

For reasons which are unclear, attempting to change any parameter on NDXOSIRIS led to:

*2024-11-13 11:00:52.352 [main] ERROR uk.ac.stfc.isis.ibex.ui.dae.experimentsetup.ExperimentSetup$1 - Index 20 out of bounds for length 20
    Index 20 out of bounds for length 20
    Stack Trace:
    java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:64)
    java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:70)
    java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:266)
    java.base/java.util.Objects.checkIndex(Objects.java:359)
    java.base/java.util.ArrayList.get(ArrayList.java:427)
    uk.ac.stfc.isis.ibex.ui.dae.experimentsetup.DaeExperimentSetupTableViewer.ifCellValueDifferentFromCachedValueThenChangeLabel(DaeExperimentSetupTableViewer.java:166)
    uk.ac.stfc.isis.ibex.ui.dae.experimentsetup.DaeExperimentSetupTableViewer.ifTableValuesDifferentFromCachedValuesThenChangeLabels(DaeExperimentSetupTableViewer.java:265)
    uk.ac.stfc.isis.ibex.ui.dae.experimentsetup.PanelViewModel.ifTableViewerValuesDifferentFromCachedValueThenChangeLabel(PanelViewModel.java:338)
    uk.ac.stfc.isis.ibex.ui.dae.experimentsetup.timechannels.TimeRegimeView.ifTableValueDifferentFromCachedValueThenChangeLabel(TimeRegimeView.java:167)
    uk.ac.stfc.isis.ibex.ui.dae.experimentsetup.timechannels.TimeChannelsPanel.ifWidgetValueDifferentFromCachedValueThenChangeLabel(TimeChannelsPanel.java:408)
    uk.ac.stfc.isis.ibex.ui.dae.experimentsetup.ExperimentSetup.changeLabelsIfDifferentFromCachedValues(ExperimentSetup.java:316)
    uk.ac.stfc.isis.ibex.ui.dae.experimentsetup.ExperimentSetup.applyChangesToUI(ExperimentSetup.java:308)
    uk.ac.stfc.isis.ibex.ui.dae.experimentsetup.ExperimentSetup$1.widgetSelected(ExperimentSetup.java:125)
    org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:252)
    org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
    org.eclipse.swt.widgets.Display.sendEvent(Display.java:4256)
    org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1066)
    org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4054)
    org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3642)
    org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1155)
    org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338)
    org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1046)
    org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:155)
    org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:643)
    org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338)
    org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:550)
    org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:171)
    uk.ac.stfc.isis.ibex.e4.product.Application.start(Application.java:65)
    org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:203)
    org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:136)
    org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
    org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:402)
    org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255)
    java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
    java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    java.base/java.lang.reflect.Method.invoke(Method.java:568)
    org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:659)
    org.eclipse.equinox.launcher.Main.basicRun(Main.java:596)
    org.eclipse.equinox.launcher.Main.run(Main.java:1467)

This is all to do with the (rather complex/hard-to-understand) mechanism we have for highlighting DAE changes which have not yet been applied.

This ticket is to try to make the DAE view as a whole more robust against this type of error (acknowledging that this exact error will be hard to reproduce):

  • Check the logic to do with highlighting changes - it is currently very complicated, can we simplify it?
  • Add much more logging/diagnostics
  • Make sure LoggerUtils.logErrorWithStackTrace is used when catching errors so we get a full stack trace
  • Make sure updating UI elements does not cause actually setting the DAE to fail

Reproducible (Yes/No)?

No

  • This was working ok from my local machine pointing at OSIRIS (gave myself write access to test)
    • Also true from release_15.0.0 branch, which is exactly the code OSIRIS have deployed
  • Tried redeploying GUI to osiris
  • Tried older guis that had been working on OSIRIS in previous cycles
  • Tried restarting DAE ioc
  • Tried restarting ISISICP
  • Tried restarting ISISICP and removing recovery.run
  • Tried restarting all of ibex server
  • Tried restarting NDX

None of the above helped on OSIRIS.

Additional Information

Acceptance Criteria

This ticket is to try to make the DAE view as a whole more robust against this type of error (acknowledging that this exact error will be very hard to reproduce):

  • Check the logic to do with highlighting changes
  • Add much more logging/diagnostics
  • Make sure LoggerUtils.logErrorWithStackTrace is used when catching errors so we get a full stack trace
  • Make sure updating UI elements does not cause actually setting the DAE to fail

How to Review

Before making a PR...

  • Provide verbose instructions for the reviewer to test that your changes work and fix the issue
  • Describe if/how you have implemented testing for this issue
  • Provide screenshots of the feature to help the reviewer if relevant

If not applicable, write "Not applicable"

...

To the reviewer: Make sure to update submodules!

Time in recording of planning meeting: 00h20m 2024/11/28
@ISISBuilder ISISBuilder moved this to Backlog in PI_2024_08 Nov 13, 2024
@KathrynBaker
Copy link
Member

Just to be certain, and to confirm priorities, has this been seen on any other instruments yet?

@Tom-Willemsen
Copy link
Contributor Author

No - we did check other instruments and this behaviour was not seen.

It is likely to be some peculiarity of OSIRIS' DAE setup which caused this. But this ticket I think should mostly be about generally hardening/logging more in this part of the UI, so that a similar problem is easier to debug next time, rather than trying to chase the exact stack trace.

@Tom-Willemsen Tom-Willemsen mentioned this issue Nov 13, 2024
Draft
@iangillingham-stfc iangillingham-stfc changed the title GUI dae: make more robust GUI dae: make more robust [Timebox 1.5 days] Nov 28, 2024
@davidkeymer davidkeymer added the 5 label Nov 28, 2024
@davidkeymer
Copy link
Contributor

Possibly disable functionality as an interim solution, if not too involved.

@Chsudeepta Chsudeepta self-assigned this Dec 2, 2024
@Chsudeepta Chsudeepta linked a pull request Dec 9, 2024 that will close this issue
Open
6 tasks
@Chsudeepta Chsudeepta added the no_release_notes Tickets that do not need release notes, use sparingly! label Dec 9, 2024
@Chsudeepta
Copy link
Contributor

The PR addresses the exception that was coming. The complicated logic of highlighting unsaved changes has been left untouched.

@ISISBuilder ISISBuilder moved this from Backlog to Review in PI_2024_08 Dec 9, 2024
@KathrynBaker KathrynBaker added the for release Required for next release label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Chsudeepta Chsudeepta

Labels
5 for release Required for next release no_release_notes Tickets that do not need release notes, use sparingly! review
Projects
PI_2024_08
Status: Review
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix for array index out of bounds ISISComputingGroup/ibex_gui
4 participants
@davidkeymer @KathrynBaker @Tom-Willemsen @Chsudeepta