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

feat(mobile): adds crop and rotate to mobile #10989

Merged
merged 26 commits into from
Jul 28, 2024

Conversation

Yuvi-raj-P
Copy link
Contributor

@Yuvi-raj-P Yuvi-raj-P commented Jul 10, 2024

Added an image editor and it has a crop feature only (for now)
Screenshot_2024-07-08-23-55-36-326_app.alextran.immich.debug-edit.jpg

Screenshot_2024-07-08-23-51-03-915_app.alextran.immich.debug-edit.jpg

@opbod
Copy link
Contributor

opbod commented Jul 10, 2024

Cropping (and image editing such as rotating/flipping) are highly requested features, thanks!

Other PRs related to this: #3271 and #9575.

This was referenced Jul 10, 2024
Copy link
Contributor

@martyfuhry martyfuhry left a comment

Choose a reason for hiding this comment

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

This is so cool, thank you for your pull request! I was not aware of the crop_image package. I'm not super familiar with this package, so I'm not completely sure about how it works.

I'd prefer using the flutter_hooks package just to remain consistent (although I've had my fair share of issues with it, in the past...)

I did not run this or test this.

mobile/lib/pages/editing/crop.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/crop.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/crop.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/crop.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/crop.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/crop.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/crop.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/edit.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/edit.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/edit.page.dart Outdated Show resolved Hide resolved
@Yuvi-raj-P
Copy link
Contributor Author

Thank you @martyfuhry for the code suggestions, I will immediately make the necessary changes. Before I approve the code I would like to test them on my device, just to make sure everything works. Thanks for those suggestions again!

Copy link
Contributor

@martyfuhry martyfuhry left a comment

Choose a reason for hiding this comment

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

I'm not tremendously sure how to add an image to the server...

I think you need to actually add the image to your own gallery on your phone, first. Then, you can manually invoke the upload procedure on the new asset.

We use photo_manager for accessing the gallery. Try the PhotoManager.editor.saveImage function? Maybe some others have a better idea.

mobile/lib/pages/editing/crop.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/crop.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/edit.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/edit.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/edit.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/edit.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/edit.page.dart Show resolved Hide resolved
Copy link
Contributor

@martyfuhry martyfuhry left a comment

Choose a reason for hiding this comment

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

Please add comments (even just one short sentence) above all of your classes to explain what they are for. Use the dart comments conventions.

It all looks good to me, otherwise! Please try saving the image to the gallery using the photo_manger package:

You can create an entity from raw data, such as downloaded images, recorded videos, etc. The created entity will show as a corresponding resource on your device's gallery app.

final Uint8List rawData = yourRawData;

// Save an image to an entity from `Uint8List`.
final AssetEntity? entity = await PhotoManager.editor.saveImage(
  rawData,
  title: 'write_your_own_title.jpg', // Affects EXIF reading.
);

We could also use the returned entity to start a backup job immediately. Or we could do that in a future PR.

mobile/lib/pages/editing/edit.page.dart Outdated Show resolved Hide resolved
@opbod
Copy link
Contributor

opbod commented Jul 23, 2024

@martyfuhry does this now acceptable for merging? Keen to see such feature!

@KweezyCode

This comment was marked as spam.

@Yuvi-raj-P
Copy link
Contributor Author

Sorry lol, not sure because it's not in my hands now for merging. @martyfuhry is going to merge it soon 😁

Copy link
Contributor

@martyfuhry martyfuhry left a comment

Choose a reason for hiding this comment

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

I am sorry for my late reply! I had forgotten about the routing, it is a small change. I will test this out on my phone soon, too!

Thank you so much for your continued patience and hard work. We really appreciate your contribution, and I know this is a very valuable feature. I am looking forward to trying this out.

mobile/lib/pages/editing/crop.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/crop.page.dart Outdated Show resolved Hide resolved
mobile/lib/pages/editing/edit.page.dart Outdated Show resolved Hide resolved
@Yuvi-raj-P
Copy link
Contributor Author

Yuvi-raj-P commented Jul 25, 2024

Hi @martyfuhry! Thanks for the suggestions, so firstly I cant replace the edit page and directly go to the crop page because of some logic in the edit page that helps the image load from the provider. The edit page is where the image is loaded from the server and displayed so removing it would mess some stuff up on the crop page. Below are the change made for my latest PR:

  1. I just labeled the Crop button with a text under the icon and also made changes so that it takes action when no edits are made.
  2. I don't have any information on how the asset gallery works so yeah, until @fyfrey replies I cant display the asset on the timeline. But all the editing images will save after some time because I am saving the image locally on the device, so when the background backup is invoked it should display the image in the server. Because I am also testing the app on the demo Immich server I cant experiment with it😭

Let me know any changes, Thanks for reviewing! Hoping to get this pr out as soon as possible 😊

@fyfrey
Copy link
Contributor

fyfrey commented Jul 26, 2024

I suggest you simply trigger a scan of the local albums/assets after saving the edited image.
Should be albumProvider.notifier.getDeviceAlbums()

This adds the new image to the timeline. To show it also in the gallery without reopening, look at what the delete button in the gallery does (it does some clearing/reset of the gallery state to refresh it from the timeline asset provider)

@Yuvi-raj-P
Copy link
Contributor Author

Yuvi-raj-P commented Jul 26, 2024

I suggest you simply trigger a scan of the local albums/assets after saving the edited image.
Should be albumProvider.notifier.getDeviceAlbums()

This adds the new image to the timeline. To show it also in the gallery without reopening, look at what the delete button in the gallery does (it does some clearing/reset of the gallery state to refresh it from the timeline asset provider)

Got it, will get that added to the PR tomorrow morning!

@Yuvi-raj-P
Copy link
Contributor Author

Done! Just got everything cleaned up and fixed. Everything should work well and the timeline gets immediately updated when saving an edited image. I hope this is the final commit, that set don't hesitate to let me know any issues with my code. Once I get this basic edit layout stuff done, I could start working on the other features like filters and doodling!

@alextran1502
Copy link
Contributor

alextran1502 commented Jul 26, 2024

Hello, I just tested the PR. Thank you so much for your contribution, great work. Below is some of my feedback.

  • When viewing the app in light mode, the edit page isn't styled accordingly yet. Here is what I see.
Entering Edit Mode Crop Mode
image image
  • I think it will be clearer to add the text to this action here as "Save to gallery."
    image
  • The pop-up message when successfully saving the edit to the gallery is blended with the bottom app bar and disappears quite quickly; maybe we can move it to the center or the top of the screen and increase the timeout a bit.
  • The action button hit area is also a little too small to tap on; maybe we can increase the radius a bit.
    When in edit mode, would it be possible to extend the editing asset to the edge of the screen?

Let me know if you need help with any of those suggestions.

@Yuvi-raj-P
Copy link
Contributor Author

Yuvi-raj-P commented Jul 26, 2024

Done! Below are the new light mode screen UI
1Screen
2Screen
3Screen

Copy link
Contributor

@alextran1502 alextran1502 left a comment

Choose a reason for hiding this comment

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

It is getting there!!!! One last request. When there is no edit made, can we hide the "Save to gallery" button"

@Yuvi-raj-P
Copy link
Contributor Author

Done! the button should be hidden until you make a change or use crop.

@alextran1502 alextran1502 enabled auto-merge (squash) July 28, 2024 20:32
@alextran1502 alextran1502 changed the title feat: Adds crop and rotate to mobile feat(mobile): adds crop and rotate to mobile Jul 28, 2024
@alextran1502 alextran1502 enabled auto-merge (squash) July 28, 2024 20:32
@alextran1502 alextran1502 merged commit 1550378 into immich-app:main Jul 28, 2024
23 checks passed
@KweezyCode
Copy link

congratulations! 😊

@opbod
Copy link
Contributor

opbod commented Jul 29, 2024

@Yuvi-raj-P many thanks for your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants