Skip to content

RMET-4099 :: added chooseFromGallery feature#1

Open
OS-ruimoreiramendes wants to merge 60 commits intofeat/RMET-4099/camera-unificationfrom
feat/RMET-4099/gallery
Open

RMET-4099 :: added chooseFromGallery feature#1
OS-ruimoreiramendes wants to merge 60 commits intofeat/RMET-4099/camera-unificationfrom
feat/RMET-4099/gallery

Conversation

@OS-ruimoreiramendes
Copy link

No description provided.

OS-ruimoreiramendes and others added 26 commits March 5, 2026 18:51
For better separation and testability
Removing unused files from example-app
Otherwise the camera pages would become extremely large
Otherwise pwa functionality is limited
OS-pedrogustavobilro and others added 27 commits March 12, 2026 12:27
Possible issue in saving to file and calling EditURIPhoto on Android with Capacitor plugin, still to be investigated.
To make sure there's no issue with edge-to-edge
@OS-pedrogustavobilro OS-pedrogustavobilro self-requested a review March 16, 2026 16:06
Copy link
Contributor

@OS-pedrogustavobilro OS-pedrogustavobilro left a comment

Choose a reason for hiding this comment

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

Overall everything looks good! From what I've been testing these days with the example app the new methods seem to be working well!

Just leaving a comment regarding something that I noticed, but it's non-blocker and we can change later so I'll approve the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a big deal but: Correct me if I'm wrong but could be a data class instead with val fields in constructor instead of var. Or are fields changing at later dates, and if so why?

Choose a reason for hiding this comment

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

From what I can see, the values are not changed later, they are loaded once and not modified afterwards. So I think it would be fine to apply the change you suggested.
That said, I followed this approach because it’s the same we already use in CameraSettings. Do you think it still makes sense to change it here as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that, and I think VideoSettings too. So if we change one, we should change the others. Whether this needs to be done in this PR or not, I'll leave that up to you 😄

Choose a reason for hiding this comment

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

Ok, I will change it in this PR

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