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

Skyline: Improvements to tutorial screenshots based on testing #3283

Merged
merged 4 commits into from
Dec 18, 2024

Conversation

brendanx67
Copy link
Contributor

  • Created ScreenshotProcessingExtensions to hold bitmap processing functions
  • Added diff coloring to the old image in ScreenshotPreviewForm
  • Added labels and a picture-box to ScreenshotPreviewForm for improved diffing
  • Improved code to decide when to use the skyline icon as Form.Icon
  • New DigitalRune.Windows.Docking.dll with better LArrow cursor
  • Hid Source tab in DocumentGridViewContext for screenshots in debug builds
  • Initial attempt to freeze the AllChromatogramsGraph for a screenshot
  • Added function to ImportFastaControl to scroll FASTA file path text to show the file
  • Hid the GPF checkbox in ImportResultsDIAControl in screenshot mode for 24.1 screenshots
  • Better normalization of language names in SkylineTester and TestRunner
  • Show context menu selection in s-09 for TestHiResMetabolomicsTutorial
  • Fixed SkylineTester issue with test selection when "Tutorials" starts selected
  • Fixed column widths in library build tab in DIA/SWATH tutorial for screenshot
  • Fixed forcing SkylineWindow on screen in DIA/SWATH tutorial during docking screenshots before docking starts
  • Fixed waits in auto-screenshot mode to avoid blurry screenshots
  • Fixed a number issues with border calculations between normal Windows forms, floating dockable forms, and docked dockable forms in Document and non-Document states.
  • Fixed screenshot numbers in URLs to match the what is currently in https://skyline.ms/tutorials/24-1/
  • Fixed some necessary border clean-up in cases where image processing is implemented

- Created ScreenshotProcessingExtensions to hold bitmap processing functions
- Added diff coloring to the old image in ScreenshotPreviewForm
- Added labels and a picture-box to ScreenshotPreviewForm for improved diffing
- Improved code to decide when to use the skyline icon as Form.Icon
- New DigitalRune.Windows.Docking.dll with better LArrow cursor
- Hid Source tab in DocumentGridViewContext for screenshots in debug builds
- Initial attempt to freeze the AllChromatogramsGraph for a screenshot
- Added function to ImportFastaControl to scroll FASTA file path text to show the file
- Hid the GPF checkbox in ImportResultsDIAControl in screenshot mode for 24.1 screenshots
- Better normalization of language names in SkylineTester and TestRunner
- Show context menu selection in s-09 for TestHiResMetabolomicsTutorial
- Fixed SkylineTester issue with test selection when "Tutorials" starts selected
- Fixed column widths in library build tab in DIA/SWATH tutorial for screenshot
- Fixed forcing SkylineWindow on screen in DIA/SWATH tutorial during docking screenshots before docking starts
- Fixed waits in auto-screenshot mode to avoid blurry screenshots
- Fixed a number issues with border calculations between normal Windows forms, floating dockable forms, and docked dockable forms in Document and non-Document states.
- Fixed screenshot numbers in URLs to match the what is currently in https://skyline.ms/tutorials/24-1/
- Fixed some necessary border clean-up in cases where image processing is implemented
Copy link
Collaborator

@eduardo-proteinms eduardo-proteinms left a comment

Choose a reason for hiding this comment

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

nice, looks good to me

Copy link
Contributor

@nickshulman nickshulman left a comment

Choose a reason for hiding this comment

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

This looks good to me, but why is there code that locks on Bitmap objects? Are Bitmap objects somehow not thread-safe?

When you need to take a screenshot of a progress indicator with a specific progress value, I wonder whether it would have been easier to tell the thing to draw itself with a particular progress value, instead of waiting until it actually gets to that number.

icon1.Save(ms1);
icon2.Save(ms2);

return StructuralComparisons.StructuralEqualityComparer.Equals(ms1.ToArray(), ms2.ToArray());
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually use "SequenceEqual" as in:
ms1.ToArray().SequenceEqual(ms2.ToArray())

@brendanx67
Copy link
Contributor Author

brendanx67 commented Dec 17, 2024

This looks good to me, but why is there code that locks on Bitmap objects? Are Bitmap objects somehow not thread-safe?

When you need to take a screenshot of a progress indicator with a specific progress value, I wonder whether it would have been easier to tell the thing to draw itself with a particular progress value, instead of waiting until it actually gets to that number.

Yeah. We modify the pixels of the bitmaps directly in our diff code, and even when I switched to having the UI thread make a copy of the Bitmap and never giving the bitmap directly to PictureBox, the diff code was throwing exceptions in GetPixel() or SetPixel() that said the object was in use. This fixed the problem.

Maybe I could have the diff code always make a copy and never SetPixel() directly on the Bitmap itself, but it wasn't clear whether I could even have two threads requesting GetPixel() from the same bitmap at the same time. I don't know how threadsafe Bitmaps are for reading vs reading-while-writing

@brendanx67
Copy link
Contributor Author

Oops. The code already does create a new copy every time. I guess that means the answer to your question is, yes, Bitmaps are not threadsafe even for GetPixel() calls on two threads at the same time. I definitely got "in use" exceptions until I added the locks.

- save TestUtilSettings on every continue to more consistently save ScreenshotPreviewForm settings
- use IEnumerable<T>.SequencesEqual() as suggested by Nick
@brendanx67 brendanx67 merged commit 6f4a95e into master Dec 18, 2024
3 checks passed
@brendanx67 brendanx67 deleted the Skyline/work/20241217_screenshot_improvements branch December 18, 2024 04:05
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.

3 participants