Allow selecting previously uploaded image for picture upload #23072

karwosts · 2024-11-30T18:13:35Z

Proposed change

In the image upload form, allow selecting a previously uploaded image via the browse media dialog.

Requires home-assistant/core#131468

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New feature (thank you!)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example configuration

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue or discussion:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
There is no commented out code in this PR.
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

bramkragten · 2024-12-04T12:22:30Z

Love this! ❤️

We need to think what to do with images sizes, currently we upload and cut an image to size, so we know what size (or ratio) image we will get. If we allow people to select previously uploaded images these may not have the requested ratio/size.

Do we cut these images again and upload them (under the same id, with a different size?)
Do we limit the selectable images to the specified ratio
Do we update the use of the images to always work no matter the size of the image?

1 use-case where this will happen is the user profile picture.

karwosts · 2024-12-05T01:22:04Z

A sequence like the following might work, for image uploads with cropOptions:

Allow choosing image from media browser as currently
On pick, load that image_upload original into the cropper dialog and present the dialog.
If the aspect ratio of the image matches the aspect ratio of cropOptions, add a button in the dialog next to Crop that says Use Original.
If "use original" is picked, we don't upload anything new and instead just keep using the existing media id from image_upload.
If aspect ratio does not match desired, or if use original is not pressed, perform the required crop (as we do on new uploads), and upload the result into a new image_upload file.

Kind of hacked my way through this and I think all the pieces work.

karwosts · 2024-12-08T05:11:10Z

Updated cropping as proposed.

wendevlin · 2024-12-12T11:53:17Z

Updated cropping as proposed.

Thank you. We wait on a design for the select prev image button and come back to you

wendevlin · 2024-12-17T16:11:00Z

Hi @karwosts,

We would like to integrate the "select media" button into the ha-file-upload

Here are designs:

Please make it optional, because we have file uploads that don't need the select option.

If you have questions just ask

karwosts · 2024-12-17T18:46:56Z

I did hesitate in my original proposal to use the term "select from media" as we are not allowing (currently) selecting any media image, it can only be a file previously uploaded via image_upload, not anything from any other media integration.

Just want to be clear you're aware of that limitation.

wendevlin · 2024-12-17T21:07:12Z

I did hesitate in my original proposal to use the term "select from media" as we are not allowing (currently) selecting any media image, it can only be a file previously uploaded via image_upload, not anything from any other media integration.

Just want to be clear you're aware of that limitation.

Ok good point, but I think we can risk it.

karwosts · 2024-12-18T00:29:37Z

Moved select media button into secondary text:

marcinbauer85 · 2024-12-18T06:58:17Z

As for the design of the picker, I've done a very similar one for choosing local image via the Local path or web URL:
https://www.figma.com/design/AKeKccgRBaCy871REB5V9c/HA-04?node-id=2008-4840&t=oTfqhrYVnfiNFrWQ-11

If the first option now gets the ability to choose a local image, the second option right now is getting redundant. It only has the ability to paste in a web URL which is not aligned with HA principles of being local only. We'll discuss this internally, and get back as soon as possible.

karwosts · 2024-12-18T13:33:10Z

Second option can also be used for local only images, for anything that anyone has manually loaded to their /www/ folder, which would have been how this has always been done up until the upload dialog was added last year.

Any files loaded manually to the /www/ folder are not available to the picker.

Or maybe for a URL which is still local but not hosted by HA.

marcinbauer85 · 2024-12-19T08:38:42Z

We could leave the Local Path via yaml, but simplify the UI to the screen below, and not have a breaking change. An added benefit is, that when uploading or selecting local media all files are accessible via the UI, while if an user uses a local URL from /www/ this later might become trash if the user doesn't clean internal files via a file browser plugin.

wendevlin · 2024-12-19T13:45:15Z

We could leave the Local Path via yaml, but simplify the UI to the screen below, and not have a breaking change. An added benefit is, that when uploading or selecting local media all files are accessible via the UI, while if an user uses a local URL from /www/ this later might become trash if the user doesn't clean internal files via a file browser plugin.

No because local path is already in the UI, removing it would be a breaking change.
I would say we go the way @karwosts did it and if we find a better solution we still can improve

wendevlin

Awesome, just some details left.

wendevlin · 2024-12-19T14:19:09Z

src/components/ha-picture-upload.ts

+      navigateIds: [
+        { media_content_id: undefined, media_content_type: undefined },
+        {
+          media_content_id: "media-source://image_upload",


please add a static constant for "media-source://image_upload"

wendevlin · 2024-12-19T14:20:41Z

src/components/ha-picture-upload.ts

+        const id = pickedMedia.item.media_content_id;
+        const stringToRemove = "media-source://image_upload/";
+        if (id.startsWith(stringToRemove)) {
+          const mediaId = id.substr(stringToRemove.length);


substr is depricated: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substr

Please use substring

wendevlin · 2024-12-19T14:21:54Z

src/components/ha-picture-upload.ts

+            const response = await fetch(url);
+            const data = await response.blob();


This can fail, please add error handling

Can you elaborate a bit what you want to see here?

So HA gives us the list of images, we click on one, and then... it's not available? What should I do with that, a popup dialog that just says "unknown error"? I would have thought just whatever default logging exception to the console would be sufficient, and how this would usually be handled.

Or what is the specific mode of failure you were thinking of?

Sounds good.

If it fails to load use showAlertDialog with a text: "Load image preview in crop editor failed" (this is just an example).

I know this is an edge case but http requests can always fail and showing it only in console won't help the user in the situation.

wendevlin · 2024-12-19T14:23:58Z

src/dialogs/image-cropper-dialog/image-cropper-dialog.ts

@@ -72,6 +95,12 @@ export class HaImagecropperDialog extends LitElement {
      <mwc-button slot="secondaryAction" @click=${this.closeDialog}>
        ${this.hass.localize("ui.common.cancel")}
      </mwc-button>
+      ${this._isTargetAspectRatio
+        ? html` <mwc-button slot="primaryAction" @click=${this._useOriginal}>


Suggested change

? html` <mwc-button slot="primaryAction" @click=${this._useOriginal}>

? html`<mwc-button slot="primaryAction" @click=${this._useOriginal}>

wendevlin · 2024-12-19T14:27:47Z

src/components/ha-picture-upload.ts

+  constructor() {
+    super();
+    this._chooseMedia = this._chooseMedia.bind(this);
+  }
+


Suggested change

constructor() {

super();

this._chooseMedia = this._chooseMedia.bind(this);

}

use arrow function instead

wendevlin · 2024-12-19T14:28:30Z

src/components/ha-picture-upload.ts

@@ -141,16 +175,58 @@ export class HaPictureUpload extends LitElement {
    }
  }

+  private _chooseMedia(): void {


Suggested change

private _chooseMedia(): void {

private _chooseMedia = () => {

home-assistant · 2024-12-19T14:29:03Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

… select-previous-upload-image

karwosts · 2024-12-19T15:40:19Z

src/components/ha-picture-upload.ts

@@ -39,13 +49,32 @@ export class HaPictureUpload extends LitElement {

  public render(): TemplateResult {
    if (!this.value) {
+      /* eslint-disable lit-a11y/anchor-is-valid */


I am not too sure about this check. Due I think to recent changes to dialogs/navigation, the href=# didn't work anymore, so I had to change to href="javascript:".

Eslint however doesn't seem to like the new "fake" anchor. I'm not sure how else to open a dialog from a link like was requested though.

Mhh yeah not the nicest solution.

What about using a button tag and style it like a link?

Allow selecting previously uploaded image for picture upload

1ce314d

home-assistant bot added the cla-signed label Nov 30, 2024

silamon added wait for backend backend merged The backend PR for this frontend PR has been merged and removed backend merged The backend PR for this frontend PR has been merged labels Dec 1, 2024

bramkragten added the Needs UX Pull requests requiring a review from the Home Assistant design team label Dec 4, 2024

karwosts added backend merged The backend PR for this frontend PR has been merged and removed wait for backend labels Dec 5, 2024

karwosts marked this pull request as draft December 7, 2024 18:08

Enable recropping if choosing previous image

a201faa

karwosts marked this pull request as ready for review December 8, 2024 05:11

wendevlin added the Noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) label Dec 12, 2024

move link into secondary

c4a6997

wendevlin requested changes Dec 19, 2024

View reviewed changes

home-assistant bot marked this pull request as draft December 19, 2024 14:29

karwosts added 2 commits December 19, 2024 06:34

Merge branch 'dev' of https://github.com/home-assistant/frontend into…

de5d995

… select-previous-upload-image

updates

5789587

karwosts commented Dec 19, 2024

View reviewed changes

karwosts added 2 commits December 19, 2024 10:43

use a button instead of a link

d87bf9a

catch fetch exception

95156b8

karwosts marked this pull request as ready for review December 19, 2024 19:34

home-assistant bot requested a review from wendevlin December 19, 2024 19:34

revert some no longer needed CSS

e10e856

karwosts removed the Needs UX Pull requests requiring a review from the Home Assistant design team label Dec 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow selecting previously uploaded image for picture upload #23072

Allow selecting previously uploaded image for picture upload #23072

karwosts commented Nov 30, 2024 •

edited

Loading

bramkragten commented Dec 4, 2024

karwosts commented Dec 5, 2024

karwosts commented Dec 8, 2024

wendevlin commented Dec 12, 2024

wendevlin commented Dec 17, 2024

karwosts commented Dec 17, 2024

wendevlin commented Dec 17, 2024

karwosts commented Dec 18, 2024 •

edited

Loading

marcinbauer85 commented Dec 18, 2024

karwosts commented Dec 18, 2024 •

edited

Loading

marcinbauer85 commented Dec 19, 2024

wendevlin commented Dec 19, 2024

wendevlin left a comment

wendevlin Dec 19, 2024

wendevlin Dec 19, 2024

wendevlin Dec 19, 2024

karwosts Dec 19, 2024

wendevlin Dec 19, 2024

wendevlin Dec 19, 2024

wendevlin Dec 19, 2024

wendevlin Dec 19, 2024

home-assistant bot commented Dec 19, 2024

karwosts Dec 19, 2024

wendevlin Dec 19, 2024

		const response = await fetch(url);
		const data = await response.blob();

	? html` <mwc-button slot="primaryAction" @click=${this._useOriginal}>
	? html`<mwc-button slot="primaryAction" @click=${this._useOriginal}>

	private _chooseMedia(): void {
	private _chooseMedia = () => {

Allow selecting previously uploaded image for picture upload #23072

Are you sure you want to change the base?

Allow selecting previously uploaded image for picture upload #23072

Conversation

karwosts commented Nov 30, 2024 • edited Loading

Proposed change

Type of change

Example configuration

Additional information

Checklist

bramkragten commented Dec 4, 2024

karwosts commented Dec 5, 2024

karwosts commented Dec 8, 2024

wendevlin commented Dec 12, 2024

wendevlin commented Dec 17, 2024

karwosts commented Dec 17, 2024

wendevlin commented Dec 17, 2024

karwosts commented Dec 18, 2024 • edited Loading

marcinbauer85 commented Dec 18, 2024

karwosts commented Dec 18, 2024 • edited Loading

marcinbauer85 commented Dec 19, 2024

wendevlin commented Dec 19, 2024

wendevlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

home-assistant bot commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karwosts commented Nov 30, 2024 •

edited

Loading

karwosts commented Dec 18, 2024 •

edited

Loading

karwosts commented Dec 18, 2024 •

edited

Loading