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

Add support for display docs and videos (same behaviour as J4) in the view used by media field #19967

Closed
wants to merge 3 commits into from

Conversation

ggppdk
Copy link
Contributor

@ggppdk ggppdk commented Mar 23, 2018

Pull Request for Issues #10871 , #17475 , #19954

Summary of Changes

Added template files for documents and videos to the com_media view (imageslist) used by media field
Added template overrides to backend isis template
Added template overrides to frontend protostar template

Testing Instructions

  1. (At Media Manager options)
    Add pdf,mp4,PDF,MP4 to Legal Extensions
    Add application/pdf,video/mp4 to Legal MIME types

  2. Open an article and go to TAB image and links, click to select an image

  3. Upload a PDF file and an MP4 file, click on each of them so that they get selected and click "Insert"

Expected result

The files are uploaded and shown and insertable

Actual result

The files are uploaded but not shown / not insertable

Documentation Changes Required

YES


$this->docs = &$docs;
$this->videos = &$videos;
$this->videos = &$videos;
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes duplicate assignment, removed it

@Quy
Copy link
Contributor

Quy commented Mar 23, 2018

For image fields, PDFs should not be displayed/selectable.

With the PR, the PDF is selectable, but won't display on the frontend to be clickable.

<div class="pull-left item-image"> <img
			 src="/joomla-cms-staging/images/test.pdf" alt="" itemprop="image"/> </div>

@ggppdk
Copy link
Contributor Author

ggppdk commented Mar 23, 2018

For image fields, PDFs should not be displayed/selectable.
With the PR, the PDF is selectable, but won't display on the frontend to be clickable.

Yes non-images should not be selectable in the image fields

Using the image fields (of images and links TAB)
is just an easy example that this PR works

The selection for non-images for specific cases should be disallowed
but before i would add anything more i would like to see if there is interest in this PR and suggestions on how to limit the view

In my own views i am using ACL + a configuration id that corresponds to the configuration data in the DB to decide what is shown / not shown, and also do other customization on the view

/**
* Set the active video
*
* @param integer $index Image position
Copy link
Contributor

Choose a reason for hiding this comment

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

Video?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, thanks

/**
* Set the active document
*
* @param integer $index Image position
Copy link
Contributor

Choose a reason for hiding this comment

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

Document?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, thanks

*
* @return void
*
* @since 1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

__DEPLOY_VERSION__

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, thanks

*
* @return void
*
* @since 1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

__DEPLOY_VERSION__

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, thanks

@@ -47,8 +47,10 @@
);
}
?>
<?php if (count($this->images) > 0 || count($this->folders) > 0) : ?>
<?php if (count($this->docs) > 0 || count($this->videos) > 0 || count($this->images) > 0 || count($this->folders) > 0) : ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

How about moving count($this->docs) > 0 || count($this->videos) > 0 to the end since images are more likely to exist than docs and videos?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, and i try to always remember for code that will be repeated

but in this case, it is only executed once per imagelist view opening,
thus the benefit would be only a couple of microseconds ... or less,
so i went for the order that the $this variables are defined in view.html.php,

or we could use the order the order that $this variables are used inside the IF:
folders, images, videos, docs

Also inside the IF the use of echo $this->loadTemplate('folder|image|video|doc'); inside loops
is really thousand times slower than this optimization, but even the above is hardly measurable because it is executed only a few dozens or hundrend times

@ggppdk ggppdk force-pushed the media_field_doc_video_support branch from 963c05b to da879c3 Compare April 1, 2018 05:40
@laoneo
Copy link
Member

laoneo commented Apr 5, 2018

Can you do this feature against the Joomla 4 branch? Don't want to introduce a new feature in J3 which doesn't exist in 4, better to do it the other way around. Thanks!

@robbiejackson
Copy link

Hi, improvement this is really useful! I was diverted here after raising a change request #20057 but I'm not sure if the functionality is the same. Would you mind comparing the text of that issue?

In particular, can I just check if this change provides access to all types of media? I recently developed a website with media which included .gpx files (XML format files of GPS tracks), so would those be visible for example?

Also, I suggested being able to filter on the types of media shown – eg by MIME type or file extension.

Many thanks!

@ggppdk
Copy link
Contributor Author

ggppdk commented Apr 14, 2018

@laoneo

Can you do this feature against the Joomla 4 branch? Don't want to introduce a new feature in J3 which doesn't exist in 4, better to do it the other way around. Thanks!

this PR so far only adds the listing of doc / video files without any configuration
(the above is done as long as legal extensions / MIME are added to com_media configuration)
and this is already possible in J4

so you mean
implement configuration of which files should be allowed per instance of media field as a PR to 4.0-dev branch and close this one ? (this has not been added to this PR --yet--)

@ggppdk
Copy link
Contributor Author

ggppdk commented Apr 14, 2018

@robbiejackson

you do not need wait for this PR to do it in any J3 installation

you can do it today, just add 2 lines

$docs    = $this->get('documents');
$videos  = $this->get('videos');

to your Joomla template override of
administrator/components/com_media/views/imageslist/tmpl/default.php
as
templates/JTEMPLATENAME/html/com_media/imageslist/default.php
administrator/templates/JTEMPLATENAME/html/com_media/imageslist/default.php

**and then add 2 loops in it **
1 loop to list documents and 1 loop to list videos

copy code for the 2 loops from here and correct the variable name of each loop
https://github.com/joomla/joomla-cms/pull/19967/files#diff-fac76ead6ee179d0a6802a886ffd84b2
https://github.com/joomla/joomla-cms/pull/19967/files#diff-64721eb2e5a153801fced8b9bae84016

if you need more help to do the above then please ask in Joomla forums or in PHP programming forums

[EDIT]
of course add the
add additional legal extensions/mime types into com_media configuration

@robbiejackson
Copy link

@ggppdk
many thanks, I'll look into that,
Robbie

@Quy
Copy link
Contributor

Quy commented May 30, 2018

Escape filename like in #20616

@Max00
Copy link

Max00 commented Jun 6, 2018

Do you think this PR will merge soon ?

@ghost
Copy link

ghost commented Jun 6, 2018

@Max00 every PR need 2 successfully Test, before a Maintainer decide to merge or not.

So if you wan't to help, please test.

@carlitorweb
Copy link
Member

@ggppdk will be good, now you add this great feature, the xtd-image button can do the same. Where the button put a link to the PDF (in case user select a PDF) or the <video></video> tag in case user select an .mp4 file.

@infograf768
Copy link
Member

@ggppdk
Would your PR solve #20969 ?

@ggppdk
Copy link
Contributor Author

ggppdk commented Jul 4, 2018

@infograf768

yes, but still this PR does not solve (yet) the configuration issue

You see there is the topic of how the MVC will decide that browsing docs and videos is allowed

Always showing all files

  • is not an option at all, since you will be listing all media files when someone only wants to view and select images

Using (-only-) a parameter in url

  • is not the way,since all existing websites after a joomla update, will suddenly allow this,

and someone by just by adding a parameter to the URL will be able to browse docs and videos in the media folder, before someone would need to know the filename for accessing a file

i suggest that the XTD button proposed in #20969
should have configuration in it

  • doc folder root for browsing doc files, which file extensions to list, etc and user-group(s) (or access level needed) for browsing

then you would add to the mediaelements URL
&conf_ext=editor_xtd&ext_id=NN

then you would read configuration from the plugin
and display will happen according to the configuration in the plugin and according to the access level (or user-groups) of current user

@carlitorweb
Copy link
Member

@ggppdk we can test this PR, or you need still work in something more? I ask because of this:

but still this PR does not solve (yet) the configuration issue

So far, this PR only add the necessary layout for add that type of files, and if is all this PR will do, then we can test it, for see if we can have merged it. After that, we can use part of this code, in the xtd button feature

@ggppdk
Copy link
Contributor Author

ggppdk commented Jul 16, 2018

Thanks anyone spending time on this PR
No interest in spending more on it by me

@ggppdk ggppdk closed this Jul 16, 2018
@Robdebert
Copy link

@ggppdk Thank you so much for your code. I was able to use it completely as an override, without having to modify core-files.
That solved my problem with the media-manager in a modal-overlay (when using it as a field-type "media") and still my joomla can be updated :-)

@ghost
Copy link

ghost commented Sep 13, 2018

@Robdebert your Comment on #19954 is resolve by your above?

@level420
Copy link

level420 commented Jan 4, 2019

@Robdebert would you mind sharing your override? Maybe as a gist? That would be great!

@Robdebert
Copy link

@level420 and @ghost: Sorry for the late reply.

I created a new issue, becuase i still think, that the media-fieldtype should use the same extensions as com_media allows, when editing the component-parameters. See #30548

Would be great, if you could support this new ticket, because the issue with the media-field is very old (i found a forum post from 2016 also).. and still Joomla-Core does not provide a modal-window fieldtype with the possibility to upload other files then images.

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.

10 participants