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.

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

GUI dae: make more robust #8568

Open
Tom-Willemsen opened this issue Nov 13, 2024 · 2 comments
Open

GUI dae: make more robust #8568

Tom-Willemsen opened this issue Nov 13, 2024 · 2 comments
Labels

Comments

@Tom-Willemsen
Copy link
Contributor

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!

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Backlog
Development

No branches or pull requests

2 participants