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

[4.0] Added Save2Copy in Media Manager #30313

Closed
wants to merge 21 commits into from

Conversation

vlaucht
Copy link

@vlaucht vlaucht commented Aug 7, 2020

Pull Request for Issue # .

Summary of Changes

Added a button to the media manager edit page to save changes as a copy, without overwriting the original file.
save2copy

The new files will be named like the original file with an added version
new_name

Testing Instructions

  1. Navigate to content -> media
  2. select an image and open the editor
  3. make some changes and click 'Save as Copy'
  4. make sure a copy of the file with the changes applied has been saved as 'original_filename-1.extension'
  5. make sure the original file has not been modified
  6. please verify that this also works in subfolders
  7. Change Plugin folder

Actual result BEFORE applying this Pull Request

All changes have been applied to the original image in a destructive way. The original image could not be restored.

Expected result AFTER applying this Pull Request

A copy of the original file is saved with changes applied, and the original file is not modified

Documentation Changes Required

Added one new method generateNewName in 'administrator/components/com_media/src/Controller/ApiController.php' and documentation is added

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Aug 7, 2020
@infograf768 infograf768 changed the title Added Save2Copy in Media Manager [4.0] Added Save2Copy in Media Manager Aug 7, 2020
@bembelimen
Copy link
Contributor

Thank you @vlaucht for your PR

@Quy
Copy link
Contributor

Quy commented Aug 7, 2020

It should be Save & Close with the dropdown item Save as Copy to be consistent with other components.

@vlaucht
Copy link
Author

vlaucht commented Aug 8, 2020

It should be Save & Close with the dropdown item Save as Copy to be consistent with other components.

I can change that, but then the save2copy option is not visible. I believe some sort of warning should be presented to the user that he is about to permanently overwrite the file if he clicks on save

@Quy
Copy link
Contributor

Quy commented Aug 8, 2020

There is no warning with the other components. It should be obvious that it is save and close per the label of the button.

@vlaucht
Copy link
Author

vlaucht commented Aug 10, 2020

There is no warning with the other components. It should be obvious that it is save and close per the label of the button.

Ok, I have swapped the order of the buttons

@infograf768
Copy link
Member

hmm. Looks like I do not get a copy at all here. No console error.
save2copy_media

@vlaucht
Copy link
Author

vlaucht commented Aug 11, 2020

hmm. Looks like I do not get a copy at all here. No console error.

Hm, just retried on a fresh 4.0 installation, and It worked as expected.

@infograf768
Copy link
Member

Tested again. The problem is with Firefox

Macintosh:
Firefox: does not work
Safari and Chrome: works fine

@infograf768
Copy link
Member

@Fedik
Any idea to solve the Firefox issue?

@vlaucht
Copy link
Author

vlaucht commented Aug 11, 2020

I just checked on that, and for some reason, it does not call the method putFile, like it does on Chrome. Possibly an issue with the edit-images.js. I will look into that later.

@richard67
Copy link
Member

Possibly an issue on Firefox side, or even some new security feature they have?

@vlaucht
Copy link
Author

vlaucht commented Aug 11, 2020

Possibly an issue on Firefox side, or even some new security feature they have?

Clicking on save does trigger a PUT request tho. Save&Close and Save2Copy seem to send a GET request for some reason

@vlaucht
Copy link
Author

vlaucht commented Aug 11, 2020

The issue is with window.location = ${pathName}?option=com_media&path=${fileDirectory}; (line 165) in
build/media_source/com_media/js/edit-images.es6.js. If this line is removed, it triggers a PUT request and everything works as expected, but then it does not navigate back of course.
Is it possible that it navigates back before the request is sent, and for that reason it sends a GET request (like it does when entering the site)?

@Fedik
Copy link
Member

Fedik commented Aug 11, 2020

who doing "request" (Joomla.UploadFile) and then change window.location? I wonder how it work at all :)

@vlaucht
Copy link
Author

vlaucht commented Aug 11, 2020

So how about sending an isClose parameter in the response and do the navigation after the request has completed?

@Fedik
Copy link
Member

Fedik commented Aug 11, 2020

the navigation after the request has completed

yeap, the navigation should be after the request completed

@infograf768
Copy link
Member

infograf768 commented Aug 11, 2020

Further tests:

Normally, Save & Copy does NOT reload the manager. See how it behaves for an article. We stay in edit form with the new article displayed in the form.
Therefore I have tested modifying js to

        forUpload.isCopy = true;
        Joomla.UploadFile.exec(name, JSON.stringify(forUpload), uploadPath, url, type);
        Joomla.MediaManager.Edit.Reset(true); // Changed here
        break;

Although we have no message telling that the modified image is saved (would be good to have), we stay in the edit form and it now works fine, waiting for a Save (Apply) or Save & Close.

I think this is the correct behavior.
Remains to add a Success message and, if we could, display the name of the file concerned (whether we use a Save or Save & Copy)

What do you think?

@Fedik
Copy link
Member

Fedik commented Aug 11, 2020

as I see, the code (php part) generate a new file name for a copied file,
so the editor need to be reloaded somehow to use that name, if I am correct

@infograf768
Copy link
Member

infograf768 commented Aug 11, 2020

This what Joomla.MediaManager.Edit.Reset(true); does, if I do not mistake.

EDIT: should do in fact. What happens is that the page url remains stuck to the original file.

@infograf768
Copy link
Member

I also tested

      case 'save2copy':
        forUpload.isCopy = true;
        Joomla.UploadFile.exec(name, JSON.stringify(forUpload), uploadPath, url, type);
        Joomla.MediaManager.Edit.Reset(true);
        window.location = "".concat(pathName, "?option=com_media&path=").concat(fileDirectory);
        break;

and it creates the copy in Firefox, then returns to media manager directly.

@vlaucht
Copy link
Author

vlaucht commented Aug 12, 2020

and it creates the copy in Firefox, then returns to media manager directly.

But navigating back right after the request was sent means it will navigate even if the request was unsuccessfull for some reason, and since there is no notification yet, the user wouldn't even notice something went wrong. I think the navigation part should be done once the request completes.

@infograf768
Copy link
Member

I agree. This is why I prefer the first solution, (#30313 (comment)) i.e. display a success message forcing to Save/Save & Close. + an error if fails.
Also display the name or path of file (easy for the existing file before save & Copy, but not so easy for the copied file)

@vlaucht
Copy link
Author

vlaucht commented Aug 12, 2020

We stay in edit form with the new article displayed in the form.

So does this mean if you click save2copy, you will now be editing the new copy instead of the original file?

@vlaucht
Copy link
Author

vlaucht commented Aug 25, 2020

@vlaucht the constructor is called only once, after the js was parsed, you have to debug the actual function eg setTitle(titleString)

@dgrammatiko I'm pretty sure I have tested it correctly.

  • I can't find the listener attached to the dom in inspector (I do find the listener registered outside the class)

  • The debugger does not stop in the setTitle(titleString) (It does stop in the onMediaFileSelected registered outside)

  • Nothing I put in the constructor is being executed

  • Neither firing with console, nor with UI does trigger onMediaFileEdit

  • The whole thing simply does not work. Again, thats the reason why I would like someone to test the commit and see if this can be reproduced

@dgrammatiko
Copy link
Contributor

The whole thing simply does not work.

Well that's probably something on your side (do you have any browser plugins?) because I shared images that this works on all browsers. Also please share your workflow how you're testing this, I've tested it both manually and also trying to edit an image using the first image in the article form. I can't help you if I don't know what you did and what expected to happen...

# Conflicts:
#	build/media_source/system/js/fields/joomla-field-media.w-c.es6.js
@dgrammatiko
Copy link
Contributor

BTW the whole js code is bloated for no reason, you don't need an event for the title, eg in the edit and image.vue you can check if the view is modal (there's a state variable for that AFAIK, I added it) and set the title directly there no need for all this complex ritual...

@vlaucht
Copy link
Author

vlaucht commented Aug 25, 2020

Also please share your workflow how you're testing this

Clone this branch into htdocs folder, run composer install and npm ci, clear browser chache, set up the joomla site, install test sample data,
navigate to frontend -> single article -> australian parks ->edit -> CMS content
Open debugger, set breakpoints, check with UI and console
Tested with firefox on windows, only extension is xdebug

@dgrammatiko
Copy link
Contributor

I think I was pretty clear many comments ago: you need to open the images modal and then check the debugger😉

@vlaucht
Copy link
Author

vlaucht commented Aug 25, 2020

I think I was pretty clear many comments ago: you need to open the images modal and then check the debugger😉

Well thats what I meant with ->CMS content. Let me clarify then: CMS content -> Image

I thought this is self explainatory, since the image modal is what is being tested here and CMS content is just an option list.

@dgrammatiko
Copy link
Contributor

Ok you're missing most of the code replace the media field with:

((customElements, Joomla) => {
  if (!Joomla) {
    throw new Error('Joomla API is not properly initiated');
  }

  Joomla.selectedFile = {};

  window.document.addEventListener('onMediaFileSelected', (e) => {
    Joomla.selectedFile = e.detail;
  });

  const execTransform = (resp, editor, fieldClass) => {
    if (resp.success === true) {
      if (resp.data[0].url) {
        if (/local-/.test(resp.data[0].adapter)) {
          const { rootFull } = Joomla.getOptions('system.paths');

          // eslint-disable-next-line prefer-destructuring
          Joomla.selectedFile.url = resp.data[0].url.split(rootFull)[1];
          if (resp.data[0].thumb_path) {
            Joomla.selectedFile.thumb = resp.data[0].thumb_path;
          } else {
            Joomla.selectedFile.thumb = false;
          }
        } else if (resp.data[0].thumb_path) {
          Joomla.selectedFile.thumb = resp.data[0].thumb_path;
        }
      } else {
        Joomla.selectedFile.url = false;
      }

      const isElement = (o) => (
        typeof HTMLElement === 'object' ? o instanceof HTMLElement
          : o && typeof o === 'object' && o !== null && o.nodeType === 1 && typeof o.nodeName === 'string'
      );

      if (Joomla.selectedFile.url) {
        if (!isElement(editor) && (typeof editor !== 'object')) {
          Joomla.editors.instances[editor].replaceSelection(`<img loading="lazy" src="${Joomla.selectedFile.url}" alt=""/>`);
        } else if (!isElement(editor) && (typeof editor === 'object' && editor.id)) {
          window.parent.Joomla.editors.instances[editor.id].replaceSelection(`<img loading="lazy" src="${Joomla.selectedFile.url}" alt=""/>`);
        } else {
          editor.value = Joomla.selectedFile.url;
          fieldClass.updatePreview();
        }
      }
    }
  };

  /**
   * Create and dispatch onMediaFileSelected Event
   *
   * @param {object}  data  The data for the detail
   *
   * @returns {void}
   */
  Joomla.getImage = (data, editor, fieldClass) => new Promise((resolve, reject) => {
    if (!data || (typeof data === 'object' && (!data.path || data.path === ''))) {
      Joomla.selectedFile = {};
      resolve({
        resp: {
          success: false,
        },
      });
      return;
    }

    const apiBaseUrl = `${Joomla.getOptions('system.paths').rootFull}administrator/index.php?option=com_media&format=json`;

    Joomla.request({
      url: `${apiBaseUrl}&task=api.files&url=true&path=${data.path}&${Joomla.getOptions('csrf.token')}=1&format=json`,
      method: 'GET',
      perform: true,
      headers: { 'Content-Type': 'application/json' },
      onSuccess: (response) => {
        const resp = JSON.parse(response);
        resolve(execTransform(resp, editor, fieldClass));
      },
      onError: (err) => {
        reject(err);
      },
    });
  });

  class JoomlaFieldMedia extends HTMLElement {
    constructor() {
      super();

      this.onSelected = this.onSelected.bind(this);
      this.show = this.show.bind(this);
      this.clearValue = this.clearValue.bind(this);
      this.modalClose = this.modalClose.bind(this);
      this.setValue = this.setValue.bind(this);
      this.updatePreview = this.updatePreview.bind(this);
      this.onMediaFileEdit = this.onMediaFileEdit.bind(this);
      this.setTitle = this.setTitle.bind(this);
    }

    static get observedAttributes() {
      return ['type', 'base-path', 'root-folder', 'url', 'modal-container', 'modal-width', 'modal-height', 'input', 'button-select', 'button-clear', 'button-save-selected', 'preview', 'preview-width', 'preview-height'];
    }

    get type() { return this.getAttribute('type'); }

    set type(value) { this.setAttribute('type', value); }

    get basePath() { return this.getAttribute('base-path'); }

    set basePath(value) { this.setAttribute('base-path', value); }

    get rootFolder() { return this.getAttribute('root-folder'); }

    set rootFolder(value) { this.setAttribute('root-folder', value); }

    get url() { return this.getAttribute('url'); }

    set url(value) { this.setAttribute('url', value); }

    get modalContainer() { return this.getAttribute('modal-container'); }

    set modalContainer(value) { this.setAttribute('modal-container', value); }

    get input() { return this.getAttribute('input'); }

    set input(value) { this.setAttribute('input', value); }

    get buttonSelect() { return this.getAttribute('button-select'); }

    set buttonSelect(value) { this.setAttribute('button-select', value); }

    get buttonClear() { return this.getAttribute('button-clear'); }

    set buttonClear(value) { this.setAttribute('button-clear', value); }

    get buttonSaveSelected() { return this.getAttribute('button-save-selected'); }

    set buttonSaveSelected(value) { this.setAttribute('button-save-selected', value); }

    get modalWidth() { return this.getAttribute(parseInt('modal-width', 10)); }

    set modalWidth(value) { this.setAttribute('modal-width', value); }

    get modalHeight() { return this.getAttribute(parseInt('modal-height', 10)); }

    set modalHeight(value) { this.setAttribute('modal-height', value); }

    get previewWidth() { return this.getAttribute(parseInt('preview-width', 10)); }

    set previewWidth(value) { this.setAttribute('preview-width', value); }

    get previewHeight() { return this.getAttribute(parseInt('preview-height', 10)); }

    set previewHeight(value) { this.setAttribute('preview-height', value); }

    get preview() { return this.getAttribute('preview'); }

    set preview(value) { this.setAttribute('preview', value); }

    get previewContainer() { return this.getAttribute('preview-container'); }

    // attributeChangedCallback(attr, oldValue, newValue) {}

    connectedCallback() {
      this.button = this.querySelector(this.buttonSelect);
      this.inputElement = this.querySelector(this.input);
      this.buttonClearEl = this.querySelector(this.buttonClear);
      this.modalElement = this.querySelector('.joomla-modal');
      this.buttonSaveSelectedElement = this.querySelector(this.buttonSaveSelected);
      this.previewElement = this.querySelector('.field-media-preview');

      if (!this.button || !this.inputElement || !this.buttonClearEl || !this.modalElement
        || !this.buttonSaveSelectedElement) {
        throw new Error('Misconfiguaration...');
      }

      if (Joomla.Bootstrap && Joomla.Bootstrap.initModal && typeof Joomla.Bootstrap.initModal === 'function') {
        Joomla.Bootstrap.initModal(this.modalElement);
      }

      this.button.addEventListener('click', this.show);

      if (this.buttonClearEl) {
        this.buttonClearEl.addEventListener('click', this.clearValue);
      }

      this.updatePreview();
    }

    disconnectedCallback() {
      if (this.button) {
        this.button.removeEventListener('click', this.show);
      }
      if (this.buttonClearEl) {
        this.buttonClearEl.removeEventListener('click', this.clearValue);
      }
    }

    onSelected(event) {
      event.preventDefault();
      event.stopPropagation();

      this.modalClose();
      return false;
    }

    show() {
      this.modalElement.open();

      Joomla.selectedFile = {};

      window.document.addEventListener('onMediaFileEdit', this.onMediaFileEdit);
      this.buttonSaveSelectedElement.addEventListener('click', this.onSelected);
    }

    modalClose() {
      window.document.removeEventListener('onMediaFileEdit', this.onMediaFileEdit);

      Joomla.getImage(Joomla.selectedFile, this.inputElement, this)
        .then(() => {
          Joomla.Modal.getCurrent().close();
          Joomla.selectedFile = {};
        })
        .catch(() => {
          Joomla.Modal.getCurrent().close();
          Joomla.selectedFile = {};
          Joomla.renderMessages({
            error: [Joomla.Text._('JLIB_APPLICATION_ERROR_SERVER')],
          });
        });
    }

    setValue(value) {
      this.inputElement.value = value;
      this.updatePreview();
    }

    clearValue() {
      this.setValue('');
    }

    updatePreview() {
      if (['true', 'static'].indexOf(this.preview) === -1 || this.preview === 'false' || !this.previewElement) {
        return;
      }

      // Reset preview
      if (this.preview) {
        const { value } = this.inputElement;

        if (!value) {
          this.previewElement.innerHTML = '<span class="field-media-preview-icon"></span>';
        } else {
          this.previewElement.innerHTML = '';
          const imgPreview = new Image();

          switch (this.type) {
            case 'image':
              imgPreview.src = /http/.test(value) ? value : Joomla.getOptions('system.paths').rootFull + value;
              imgPreview.setAttribute('alt', '');
              break;
            default:
              // imgPreview.src = dummy image path;
              break;
          }

          this.previewElement.style.width = this.previewWidth;
          this.previewElement.appendChild(imgPreview);
        }
      }
    }

    onMediaFileEdit(e) {
      if (e.detail && e.detail.file && e.detail.file.name) {
        this.setTitle(e.detail.file.name);
        Joomla.selectedFile = e.detail;
      }
    }

    setTitle(titleString) {
      const title = this.querySelector('.modal-title');

      if (title) {
        title.textContent = `${ Joomla.JText._('PLG_IMAGE_BUTTON_IMAGE') } - ${ titleString } `;
      }
    }
  }
  customElements.define('joomla-field-media', JoomlaFieldMedia);
})(customElements, Joomla);

and try again setting the debugger inside setTitle(titleString) and onMediaFileEdit(e)

@vlaucht
Copy link
Author

vlaucht commented Aug 25, 2020

Ok you're missing most of the code replace the media field with:

I don't see how this would change anything relevant to the problem, that the listeners are not getting registered. It just moves the methods to the end and removes some lines irrelevant to the issue. (Still tested it tho, but issue is still present)

@dgrammatiko
Copy link
Contributor

Honestly I have no clue how you try to debug this but it works fine here
Screenshot 2020-08-25 at 14 46 07
Screenshot 2020-08-25 at 14 46 21

You can try adding some console.log('whatever'); inside setTitle(titleString) and onMediaFileEdit(e) and then recompile npm run build:js

@vlaucht
Copy link
Author

vlaucht commented Aug 25, 2020

Honestly I have no clue how you try to debug this but it works fine here

We have been at this point before. I know that it works for you, it just doesnt work for me. I tried console.logs already, but as I already mentioned, the listeners are not registered, so this never gets executed. All listeners registered outside the class work like expected. Since I also tried multiple times with fresh joomla installation, there is nothing I can do besides waiting until someone tests this and can either confirm that this is an issue at my end, or that there is something else wrong.

@richard67
Copy link
Member

@vlaucht Sorry I can't help here, am not a js guy ... but I did read you use xdebug. Maybe it's an xdebug issue that the event listeners don't register, or at least an issue which happens only when using xdebug? I remember having read something similar in some other issue in past.

@dgrammatiko
Copy link
Contributor

xdebug is for PHP, for debugging JS you need to use the browser's debugger. The PHP debugger will NEVER break on any JS breakpoint...

@richard67
Copy link
Member

richard67 commented Aug 25, 2020

Silly me ;-)

@vlaucht
Copy link
Author

vlaucht commented Aug 25, 2020

@richard67 You dont need any JS knowledge. If you follow the steps in Comment and then click on the pencil icon of an image in the modal, it should open the edit view and change the name in the modal title. If not, you can open the browser debug tool, under sources navigate to media/system/fields/joomla-field-media.min.js and pretty-print it. Then find the methods onMediaFileEdit and setTitle and set a breakpoint. Then try to open the edit window and see if the debugger stops, or go to the browser console and try to call
document.dispatchEvent( new CustomEvent( 'onMediaFileEdit', { "bubbles": true, "cancelable": false, "detail": { file: { name: 'test' } }, } ) );

@mqueme
Copy link

mqueme commented Oct 17, 2020

I tested this. I click on the save as copy button, and nothing happens. no errors displayed

@mqueme
Copy link

mqueme commented Oct 17, 2020

I have tested this item 🔴 unsuccessfully on c889ca5

Tested, drop down appears but nothing happens when clicked


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30313.

@drmenzelit
Copy link
Contributor

@vlaucht interesting feature, any chance you can solve conflicts?

@bembelimen bembelimen added the Ready to take over This is an abandoned feature which can be taken over by another person to finish it. label Jan 23, 2022
@bembelimen bembelimen changed the base branch from 4.0-dev to 4.1-dev January 23, 2022 10:09
@Quy Quy added PR-4.1-dev and removed PR-4.0-dev labels Jan 28, 2022
@HLeithner HLeithner changed the base branch from 4.1-dev to 4.2-dev June 27, 2022 13:09
@HLeithner
Copy link
Member

This pull request has automatically rebased to 4.2-dev.

@Quy Quy added PR-4.2-dev and removed PR-4.1-dev labels Jun 27, 2022
@rdeutz
Copy link
Contributor

rdeutz commented Oct 21, 2022

Nice feature needs a new start, somewhere else.

@rdeutz rdeutz closed this Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Conflicting Files Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester Ready to take over This is an abandoned feature which can be taken over by another person to finish it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet