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

Screenshot preview #48

Closed
wants to merge 13 commits into from
Closed

Screenshot preview #48

wants to merge 13 commits into from

Conversation

john-rocky
Copy link
Contributor

@john-rocky john-rocky commented Jul 25, 2024 •
edited by UltralyticsAssistant
Loading

A preview is now displayed when sharing screenshots.

🛠️ PR Summary

Made with ❤️ by Ultralytics Actions

🌟 Summary

Added a new feature to allow users to share screenshots directly from the app.

📊 Key Changes

  • Incremented app version from 25 to 26 in Info.plist.
  • Introduced showShareAlert(image:), shareImage(image:), and hideScreenshotImageView() methods to manage screenshot sharing.
  • Modified screenshot capture logic to handle orientation and properly display the image before sharing.

🎯 Purpose & Impact

  • Enhanced User Experience: Users can now easily share screenshots taken within the app, improving interactive functions. 📱➡️✨
  • Streamlined Sharing: The new alert and share functions provide a smooth and intuitive sharing process, reducing steps for the user. 🔄🖼️
  • Visual Feedback: The app visually confirms screenshot actions with animations, making interactions more engaging. 👁️🎬

@john-rocky john-rocky mentioned this pull request Jul 25, 2024
Closed
@ambitious-octopus ambitious-octopus changed the title A preview is now displayed when sharing screenshots. Screenshot preview. Jul 25, 2024
ambitious-octopus
ambitious-octopus reviewed Jul 25, 2024
View reviewed changes
Copy link
Member

@ambitious-octopus ambitious-octopus left a comment

Choose a reason for hiding this comment

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

Thanks for creating a new PR @john-rocky.

ambitious-octopus
ambitious-octopus requested changes Jul 25, 2024
View reviewed changes
YOLO.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
YOLO.xcodeproj/project.pbxproj Outdated
6381D2142B7817C200ABA4E8 /* yolov8x.mlpackage */ = {isa = PBXFileReference; lastKnownFileType = folder.mlpackage; path = yolov8x.mlpackage; sourceTree = "<group>"; };
6381D2152B7817C200ABA4E8 /* yolov8s.mlpackage */ = {isa = PBXFileReference; lastKnownFileType = folder.mlpackage; path = yolov8s.mlpackage; sourceTree = "<group>"; };
6381D2162B7817C200ABA4E8 /* yolov8m.mlpackage */ = {isa = PBXFileReference; lastKnownFileType = folder.mlpackage; path = yolov8m.mlpackage; sourceTree = "<group>"; };
6381D2172B7817C200ABA4E8 /* yolov8n.mlpackage */ = {isa = PBXFileReference; lastKnownFileType = folder.mlpackage; path = yolov8n.mlpackage; sourceTree = "<group>"; };
Copy link
Member

Choose a reason for hiding this comment

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

Also here!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reason as above. If you want to restore it, I will.

Copy link
Member

Choose a reason for hiding this comment

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

Also here, keep these references!

YOLO.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
YOLO.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
YOLO/ViewController.swift Outdated Show resolved Hide resolved
@ambitious-octopus ambitious-octopus changed the title Screenshot preview. Screenshot preview Aug 12, 2024
ambitious-octopus
ambitious-octopus reviewed Aug 12, 2024
View reviewed changes
Copy link
Member

@ambitious-octopus ambitious-octopus left a comment

Choose a reason for hiding this comment

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

Hey @john-rocky, this is the next PR to integrate, can you resolve conflicts?

@john-rocky
Copy link
Contributor Author

@ambitious-octopus
I have resolved these conflicts.
This feature works with the landscape mode.

ambitious-octopus
ambitious-octopus requested changes Aug 13, 2024
View reviewed changes
Copy link
Member

@ambitious-octopus ambitious-octopus left a comment

Choose a reason for hiding this comment

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

Hey @john-rocky, please keep the references and DEVELOPMENT_TEAM in YOLO.xcodeproj/project.pbxproj . Also add a +1 on the build number. Have you tested the branch independently?

YOLO.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
YOLO.xcodeproj/project.pbxproj Outdated
6381D2142B7817C200ABA4E8 /* yolov8x.mlpackage */ = {isa = PBXFileReference; lastKnownFileType = folder.mlpackage; path = yolov8x.mlpackage; sourceTree = "<group>"; };
6381D2152B7817C200ABA4E8 /* yolov8s.mlpackage */ = {isa = PBXFileReference; lastKnownFileType = folder.mlpackage; path = yolov8s.mlpackage; sourceTree = "<group>"; };
6381D2162B7817C200ABA4E8 /* yolov8m.mlpackage */ = {isa = PBXFileReference; lastKnownFileType = folder.mlpackage; path = yolov8m.mlpackage; sourceTree = "<group>"; };
6381D2172B7817C200ABA4E8 /* yolov8n.mlpackage */ = {isa = PBXFileReference; lastKnownFileType = folder.mlpackage; path = yolov8n.mlpackage; sourceTree = "<group>"; };
Copy link
Member

Choose a reason for hiding this comment

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

Also here, keep these references!

YOLO.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
YOLO.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
YOLO.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
@john-rocky
Copy link
Contributor Author

@ambitious-octopus
I revert the model reference and increment the bundle version.
I tested this branch. This branch works fine.

ambitious-octopus
ambitious-octopus requested changes Aug 14, 2024
View reviewed changes
Copy link
Member

@ambitious-octopus ambitious-octopus left a comment

Choose a reason for hiding this comment

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

Thank you @john-rocky . Let's fix the formatting and the PR is ready!

YOLO.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
ambitious-octopus
ambitious-octopus reviewed Aug 16, 2024
View reviewed changes
Copy link
Member

@ambitious-octopus ambitious-octopus left a comment

Choose a reason for hiding this comment

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

Thanks for resolving the issue, @john-rocky . @glenn-jocher , this PR is now ready and has been thoroughly tested by @john-rocky . In summary, it fixes the button for sharing photos with bounding boxes and also adds a preview of the image before it is sent.

@glenn-jocher
Copy link
Member

@john-rocky @ambitious-octopus thanks guys, but I don't think we want an image preview. The image is already previewed in most sharing tools like WhatsApp or Slack before the user commits to sharing the image, so we're just inserting an additional step into their already complicated world.

Also I don't see any adaptation in the code for landscape, so the preview image will have the high gap top and bottom even in landscape. This is why we don't want to load this down with features and functions, this is a minimum viable product (MVP) that users can use to start building their own apps, we don't want this to be a fully feature product.

So please delete the preview and then this should be good to merge!

@glenn-jocher glenn-jocher added the TODO High priority items label Aug 16, 2024
ambitious-octopus
ambitious-octopus reviewed Aug 21, 2024
View reviewed changes
Copy link
Member

@ambitious-octopus ambitious-octopus left a comment

Choose a reason for hiding this comment

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

This PR is specifically for adding preview functionality. If we decide against this feature, we can close the PR. Does that sound right, @john-rocky ? This was implemented based on user feedback from those testing apps that currently lack a preview option.

@john-rocky
Copy link
Contributor Author

@ambitious-octopus
Yes. That's right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ambitious-octopus ambitious-octopus ambitious-octopus requested changes

Assignees

@john-rocky john-rocky

Labels
TODO High priority items
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@john-rocky @glenn-jocher @ambitious-octopus @UltralyticsAssistant