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

Handle pausing of imagery from viewLargeAction - 3647 #5901

Merged
merged 15 commits into from
Jan 20, 2023
Merged

Conversation

michaelrogers
Copy link
Contributor

@michaelrogers michaelrogers commented Oct 21, 2022

Closes #3647

Describe your changes:

This is the initial proof of concept for externalizing pause controls and providing context from the imageryView. In the current implementation the viewLargeAction preserves the pre pause/play status of the imagery. This allows the imagery to be enlarged but paused on the selected image; when closing out of the overlay the previous pause/play state will be restored.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Command line build passes?
  • Has this been smoke tested?
  • Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?
  • Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
  • Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

@codecov
Copy link

codecov bot commented Oct 21, 2022

Codecov Report

Merging #5901 (716b954) into master (70074c5) will decrease coverage by 0.11%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5901      +/-   ##
==========================================
- Coverage   54.81%   54.71%   -0.11%     
==========================================
  Files         600      600              
  Lines       23950    23971      +21     
  Branches     2117     2117              
==========================================
- Hits        13129    13116      -13     
- Misses      10219    10255      +36     
+ Partials      602      600       -2     
Flag Coverage Δ *Carryforward flag
e2e-ci 39.49% <0.00%> (ø) Carriedforward from 70074c5
e2e-full 51.23% <66.66%> (ø) Carriedforward from 70074c5
e2e-stable 50.32% <100.00%> (+0.23%) ⬆️
unit 50.24% <12.50%> (-0.31%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
src/plugins/imagery/components/ImageryView.vue 43.75% <0.00%> (ø)
src/plugins/imagery/ImageryView.js 96.42% <92.85%> (-3.58%) ⬇️
src/plugins/viewLargeAction/viewLargeAction.js 75.00% <100.00%> (+35.00%) ⬆️
src/ui/inspector/InspectorViews.vue 64.28% <0.00%> (-28.58%) ⬇️
...Conductor/independent/IndependentTimeConductor.vue 0.00% <0.00%> (-28.13%) ⬇️
src/plugins/plot/inspector/PlotOptions.vue 80.00% <0.00%> (-20.00%) ⬇️
src/plugins/plot/stackedPlot/StackedPlot.vue 52.56% <0.00%> (-8.98%) ⬇️
src/ui/inspector/Inspector.vue 75.00% <0.00%> (-4.17%) ⬇️
src/ui/layout/BrowseBar.vue 44.31% <0.00%> (-2.28%) ⬇️
src/ui/components/ObjectView.vue 46.19% <0.00%> (-2.18%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70074c5...716b954. Read the comment docs.

@michaelrogers michaelrogers changed the title MCT3647 Handle pausing of imagery from viewLargeAction Oct 31, 2022
@michaelrogers michaelrogers changed the title Handle pausing of imagery from viewLargeAction Handle pausing of imagery from viewLargeAction - 3647 Oct 31, 2022
@michaelrogers michaelrogers marked this pull request as ready for review October 31, 2022 19:42
@akhenry akhenry self-requested a review October 31, 2022 20:39
@unlikelyzero unlikelyzero added this to the 2.1.3 milestone Nov 10, 2022
@ozyx ozyx modified the milestones: Target:2.1.4, Target:2.1.5 Dec 6, 2022
@akhenry
Copy link
Contributor

akhenry commented Dec 20, 2022

Really nice, this has turned out super clean, thanks!

Copy link
Contributor

@akhenry akhenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelrogers Dang it, found a last minute bug.

  1. Expand an image by either clicking on it, or using the view large action
  2. Dismiss the overlay
  3. Attempt to expand the image again by clicking on it.
  4. Observe that the image does not get enlarged and there is an error in the JS console
vue.js?7193:1906 TypeError: viewLargeAction.onItemClicked is not a function
    at VueComponent.expand (ImageryView.vue?e345:724:1)
    at invokeWithErrorHandling (vue.js?7193:1872:1)
    at HTMLDivElement.invoker (vue.js?7193:2197:1)
    at original._wrapper (vue.js?7193:7591:1)

@michaelrogers
Copy link
Contributor Author

michaelrogers commented Dec 20, 2022

@michaelrogers Dang it, found a last minute bug.

  1. Expand an image by either clicking on it, or using the view large action
  2. Dismiss the overlay
  3. Attempt to expand the image again by clicking on it.
  4. Observe that the image does not get enlarged and there is an error in the JS console
vue.js?7193:1906 TypeError: viewLargeAction.onItemClicked is not a function
    at VueComponent.expand (ImageryView.vue?e345:724:1)
    at invokeWithErrorHandling (vue.js?7193:1872:1)
    at HTMLDivElement.invoker (vue.js?7193:2197:1)
    at original._wrapper (vue.js?7193:7591:1)

@akhenry - Really good catch, I noticed this too in testing but also observed the error was also present in master. I meant to card this up as a separate bug. It seems to be some inconsistency in the mounting/unmounting of the component click handlers.

Updated to include a fix for the above seen error. The onItemClicked method was not available on repeat triggering of the view large action and so invoke was used in its stead.

@michaelrogers
Copy link
Contributor Author

@akhenry or @shefalijoshi - this PR is ready for re-review.

@michaelrogers michaelrogers enabled auto-merge (squash) January 6, 2023 14:40
@shefalijoshi shefalijoshi removed their request for review January 9, 2023 17:34
Copy link
Contributor

@akhenry akhenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found another blocking issue unfortunately.

  1. From a display containing the image view, click on a thumbnail to select an historical image
  2. Observe that the main image changes to that of the selected thumbnail
  3. View large
  4. Observe that the large image is now showing the latest image, and not the selected one.

@michaelrogers
Copy link
Contributor Author

I've found another blocking issue unfortunately.

  1. From a display containing the image view, click on a thumbnail to select an historical image
  2. Observe that the main image changes to that of the selected thumbnail
  3. View large
  4. Observe that the large image is now showing the latest image, and not the selected one.

The logic was mistakenly hardcoded to use the latest image index when pausing which was an undesired outcome. It has been updated to maintain the focusedImageIndex when the view large action is triggered.

@michaelrogers michaelrogers merged commit e0ca620 into master Jan 20, 2023
@akhenry akhenry deleted the mct3647-v2 branch January 20, 2023 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants