Media select all #8852

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
9 participants
@okonomiyaki3000
Contributor

okonomiyaki3000 commented Jan 7, 2016

Adding a "select all" box in the media list view. At the moment, the only batch operation that can be performed in this view is "delete" so the main point of this is to make it easier when you have a folder full of things that you need to delete.

Testing

It works like any other "select all" box so, to test it, just go to a media folder that has multiple items in it and check the box. You should see that they all become selected. Uncheck the box, they will all be deselected. It works both ways so you can also check all item checkboxes and you should see that the "select all" becomes selected automatically, deselect one of the item checkboxes and the "select all" becomes deselected.

More?

It seems to me that it would be nice to be able to select not only "all" but also on the basis of type. This is almost trivial because this view already treats folders, images, videos, and other documents as separate groups. In fact, it can be done relatively easily but it doesn't work as perfectly as I'd like due to limitations of the Joomla core js. Honestly, some of those functions are just not super great. Perhaps someone will completely rewrite them in the future and then these additional features can be easily added.

@NLRoosj

This comment has been minimized.

Show comment
Hide comment
@NLRoosj

NLRoosj Jan 10, 2016

I have tested this item successfully on 27f3371


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

NLRoosj commented Jan 10, 2016

I have tested this item successfully on 27f3371


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

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jan 21, 2016

Contributor

I have tested this item 🔴 unsuccessfully on 27f3371

The select and delete work but the message is wrong
It only shows

Message
Delete Complete: /sampledata/fruitshop/tamarind.jpg

It would be better if it was to say something like "Multiple selected files deleted"


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

Contributor

brianteeman commented Jan 21, 2016

I have tested this item 🔴 unsuccessfully on 27f3371

The select and delete work but the message is wrong
It only shows

Message
Delete Complete: /sampledata/fruitshop/tamarind.jpg

It would be better if it was to say something like "Multiple selected files deleted"


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

@okonomiyaki3000

This comment has been minimized.

Show comment
Hide comment
@okonomiyaki3000

okonomiyaki3000 Jan 22, 2016

Contributor

Ah, good catch. I'll fix this.

Contributor

okonomiyaki3000 commented Jan 22, 2016

Ah, good catch. I'll fix this.

@okonomiyaki3000

This comment has been minimized.

Show comment
Hide comment
@okonomiyaki3000

okonomiyaki3000 Jan 22, 2016

Contributor

@brianteeman Actually... that has nothing to do with this PR. Multiple selection/deletion is already a thing and this poor message is the status quo. I can fix it in this PR though.

Contributor

okonomiyaki3000 commented Jan 22, 2016

@brianteeman Actually... that has nothing to do with this PR. Multiple selection/deletion is already a thing and this poor message is the status quo. I can fix it in this PR though.

@joomla-cms-bot

This comment has been minimized.

Show comment
Hide comment
@joomla-cms-bot

joomla-cms-bot Jan 22, 2016

This PR has received new commits.

CC: @brianteeman, @NLRoosj


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

This PR has received new commits.

CC: @brianteeman, @NLRoosj


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

@okonomiyaki3000

This comment has been minimized.

Show comment
Hide comment
@okonomiyaki3000

okonomiyaki3000 Jan 22, 2016

Contributor

I'm updating both the 'file' controller and the 'folder' controller to report all deleted files and folders but, in fact, the 'file' controller is never actually used. Actually, a lot of things in this component are outdated or could use some work. For example, JError is still used quite a lot.

Contributor

okonomiyaki3000 commented Jan 22, 2016

I'm updating both the 'file' controller and the 'folder' controller to report all deleted files and folders but, in fact, the 'file' controller is never actually used. Actually, a lot of things in this component are outdated or could use some work. For example, JError is still used quite a lot.

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Jan 22, 2016

Member
print_r($app); die();

😃

Member

infograf768 commented Jan 22, 2016

print_r($app); die();

😃

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jan 22, 2016

Contributor

The component is scheduled for replacement but I guess not till 3.6 now

On 22 January 2016 at 09:48, infograf768 notifications@github.com wrote:

print_r($app); die();

[image: 😃]


Reply to this email directly or view it on GitHub
#8852 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Contributor

brianteeman commented Jan 22, 2016

The component is scheduled for replacement but I guess not till 3.6 now

On 22 January 2016 at 09:48, infograf768 notifications@github.com wrote:

print_r($app); die();

[image: 😃]


Reply to this email directly or view it on GitHub
#8852 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@okonomiyaki3000

This comment has been minimized.

Show comment
Hide comment
@okonomiyaki3000

okonomiyaki3000 Jan 22, 2016

Contributor

lol! Epic fail.

Contributor

okonomiyaki3000 commented Jan 22, 2016

lol! Epic fail.

@joomla-cms-bot

This comment has been minimized.

Show comment
Hide comment
@joomla-cms-bot

joomla-cms-bot Jan 25, 2016

This PR has received new commits.

CC: @brianteeman, @NLRoosj


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

This PR has received new commits.

CC: @brianteeman, @NLRoosj


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

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jan 26, 2016

Contributor

I have tested this item successfully on 6c71696


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

Contributor

brianteeman commented Jan 26, 2016

I have tested this item successfully on 6c71696


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

@genesisfan

This comment has been minimized.

Show comment
Hide comment
@genesisfan

genesisfan Feb 19, 2016

I have tested this item successfully on 6c71696

It works like a charm!


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

I have tested this item successfully on 6c71696

It works like a charm!


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

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Feb 19, 2016

Contributor

RTC thanks


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

Contributor

brianteeman commented Feb 19, 2016

RTC thanks


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

@joomla-cms-bot joomla-cms-bot added the RTC label Feb 19, 2016

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Feb 23, 2016

Contributor

I'm sorry @okonomiyaki3000 but there are conflicts - so I can't merge it - can you rebase please?

Contributor

wilsonge commented Feb 23, 2016

I'm sorry @okonomiyaki3000 but there are conflicts - so I can't merge it - can you rebase please?

@joomla-cms-bot

This comment has been minimized.

Show comment
Hide comment
@joomla-cms-bot

joomla-cms-bot Feb 24, 2016

This PR has received new commits.

CC: @brianteeman, @genesisfan, @NLRoosj


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

This PR has received new commits.

CC: @brianteeman, @genesisfan, @NLRoosj


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

@brianteeman brianteeman modified the milestone: Joomla 3.5.2 Apr 12, 2016

@rdeutz

This comment has been minimized.

Show comment
Hide comment
@rdeutz

rdeutz Apr 13, 2016

Contributor

This is a new feature so it has to go into 3.6.x ?

Contributor

rdeutz commented Apr 13, 2016

This is a new feature so it has to go into 3.6.x ?

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Apr 13, 2016

Contributor

Personally its more of a bug fix than a feature as this should have always been there and it cant be said to have any effect in anything else.

Contributor

brianteeman commented Apr 13, 2016

Personally its more of a bug fix than a feature as this should have always been there and it cant be said to have any effect in anything else.

@rdeutz rdeutz modified the milestones: Joomla! 3.6.0, Joomla 3.5.2 Apr 13, 2016

@rdeutz rdeutz added New Feature and removed RTC labels Apr 13, 2016

@rdeutz

This comment has been minimized.

Show comment
Hide comment
@rdeutz

rdeutz Apr 13, 2016

Contributor

"It should have been there" fits for or lot of issues :-)

Anyway I checked the last commits, there are files renamed, so when someone has overwritten a layout it fails (chances are low I know). I also think it should be testet again.

If we are strict with semver then this is a interface change and allows a different way of doing something so it isn't bugfix it is a new way and so it is a new feature.


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

Contributor

rdeutz commented Apr 13, 2016

"It should have been there" fits for or lot of issues :-)

Anyway I checked the last commits, there are files renamed, so when someone has overwritten a layout it fails (chances are low I know). I also think it should be testet again.

If we are strict with semver then this is a interface change and allows a different way of doing something so it isn't bugfix it is a new way and so it is a new feature.


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

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Apr 13, 2016

Contributor

Wow - didnt actually spot that this (small change) had so many edits. Seeing all the file changes I agree with you 3.6 and will retest


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

Contributor

brianteeman commented Apr 13, 2016

Wow - didnt actually spot that this (small change) had so many edits. Seeing all the file changes I agree with you 3.6 and will retest


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

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Apr 13, 2016

Contributor

I have tested this item 🔴 unsuccessfully on 2c272f1

Came across a problem when deleting invalid files

Create a file caled test (1).jpg and add to the images folder manually

If you try to delete it you will correctly get a message
Unable to delete: . File name must only contain alphanumeric characters and no spaces.

Now try to use the delete all checkbox on the folder and you get a message
Unable to delete: .

When you try to delete multiple files you get a success message for each file with the filename as expected
But if you have multiple files that cannot be deleted like above then you only get the one message
Unable to delete: .


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

brianteeman commented Apr 13, 2016

I have tested this item 🔴 unsuccessfully on 2c272f1

Came across a problem when deleting invalid files

Create a file caled test (1).jpg and add to the images folder manually

If you try to delete it you will correctly get a message
Unable to delete: . File name must only contain alphanumeric characters and no spaces.

Now try to use the delete all checkbox on the folder and you get a message
Unable to delete: .

When you try to delete multiple files you get a success message for each file with the filename as expected
But if you have multiple files that cannot be deleted like above then you only get the one message
Unable to delete: .


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8852.
@okonomiyaki3000

This comment has been minimized.

Show comment
Hide comment
@okonomiyaki3000

okonomiyaki3000 Apr 13, 2016

Contributor

I'll have a look at that but maybe we should address the issue of being unable to delete files because of their names. Is there any reason for that?

Contributor

okonomiyaki3000 commented Apr 13, 2016

I'll have a look at that but maybe we should address the issue of being unable to delete files because of their names. Is there any reason for that?

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Apr 13, 2016

Contributor

The reason is that there has always been the view that media manager should
only delete files that it can create.

BUT as this has now been pushed to 3.6 and @dgt41 is working on a new Media
Manager for 3.6 maybe any further new work on the existing Media Manager is
pointless

@dgt41 Thoughts?

On 13 April 2016 at 07:55, Elijah Madden notifications@github.com wrote:

I'll have a look at that but maybe we should address the issue of being
unable to delete files because of their names. Is there any reason for
that?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8852 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Contributor

brianteeman commented Apr 13, 2016

The reason is that there has always been the view that media manager should
only delete files that it can create.

BUT as this has now been pushed to 3.6 and @dgt41 is working on a new Media
Manager for 3.6 maybe any further new work on the existing Media Manager is
pointless

@dgt41 Thoughts?

On 13 April 2016 at 07:55, Elijah Madden notifications@github.com wrote:

I'll have a look at that but maybe we should address the issue of being
unable to delete files because of their names. Is there any reason for
that?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8852 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@okonomiyaki3000

This comment has been minimized.

Show comment
Hide comment
@okonomiyaki3000

okonomiyaki3000 Apr 13, 2016

Contributor

It's perfectly fine with me to drop this PR if the functionality is being included in the new version.

Contributor

okonomiyaki3000 commented Apr 13, 2016

It's perfectly fine with me to drop this PR if the functionality is being included in the new version.

@okonomiyaki3000

This comment has been minimized.

Show comment
Hide comment
@okonomiyaki3000

okonomiyaki3000 Apr 13, 2016

Contributor

On the other hand, if there's a chance that the rewrite would be delayed and get pushed back to whatever version comes after 3.6, I can easily brush this up to make sure at least this feature gets in.

Contributor

okonomiyaki3000 commented Apr 13, 2016

On the other hand, if there's a chance that the rewrite would be delayed and get pushed back to whatever version comes after 3.6, I can easily brush this up to make sure at least this feature gets in.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Apr 13, 2016

Contributor

I would wait for a comment from @dgt41

On 13 April 2016 at 09:03, Elijah Madden notifications@github.com wrote:

On the other hand, if there's a chance that the rewrite would be delayed
and get pushed back to whatever version comes after 3.6, I can easily brush
this up to make sure at least this feature gets in.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8852 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Contributor

brianteeman commented Apr 13, 2016

I would wait for a comment from @dgt41

On 13 April 2016 at 09:03, Elijah Madden notifications@github.com wrote:

On the other hand, if there's a chance that the rewrite would be delayed
and get pushed back to whatever version comes after 3.6, I can easily brush
this up to make sure at least this feature gets in.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8852 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Apr 15, 2016

Contributor

Setting to Needs Review by @dgt41 ;)


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

Contributor

brianteeman commented Apr 15, 2016

Setting to Needs Review by @dgt41 ;)


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

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d May 7, 2016

Contributor

I am in the loop for the new Media Manager as well, work is being done for it but it isn't implemented until finished. Until then I would like to see this go in as it enhances the user experience.

@okonomiyaki3000 Could you please brush op this PR so we can get it tested and merged? Thank you.

P.s. if you are interested in contributing to the new media manager, let me know.


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

Contributor

roland-d commented May 7, 2016

I am in the loop for the new Media Manager as well, work is being done for it but it isn't implemented until finished. Until then I would like to see this go in as it enhances the user experience.

@okonomiyaki3000 Could you please brush op this PR so we can get it tested and merged? Thank you.

P.s. if you are interested in contributing to the new media manager, let me know.


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

@okonomiyaki3000

This comment has been minimized.

Show comment
Hide comment
@okonomiyaki3000

okonomiyaki3000 May 9, 2016

Contributor

I'll get this PR ready then. I might be interested in the new one but not sure if I have much time to work on it.

Contributor

okonomiyaki3000 commented May 9, 2016

I'll get this PR ready then. I might be interested in the new one but not sure if I have much time to work on it.

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d May 9, 2016

Contributor

@okonomiyaki3000 Thank you, I will let Jisse contact you and you guys can figure it out the options.

Contributor

roland-d commented May 9, 2016

@okonomiyaki3000 Thank you, I will let Jisse contact you and you guys can figure it out the options.

@okonomiyaki3000

This comment has been minimized.

Show comment
Hide comment
@okonomiyaki3000

okonomiyaki3000 May 9, 2016

Contributor

@brianteeman In the case that you try to delete a badly named file along with some properly named files, do you think it's best to abort all delete operations or just skip the badly named file(s)?

Contributor

okonomiyaki3000 commented May 9, 2016

@brianteeman In the case that you try to delete a badly named file along with some properly named files, do you think it's best to abort all delete operations or just skip the badly named file(s)?

@joomla-cms-bot

This comment has been minimized.

Show comment
Hide comment
@joomla-cms-bot

joomla-cms-bot May 9, 2016

This PR has received new commits.

CC: @brianteeman, @genesisfan, @NLRoosj


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

This PR has received new commits.

CC: @brianteeman, @genesisfan, @NLRoosj


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

@okonomiyaki3000

This comment has been minimized.

Show comment
Hide comment
@okonomiyaki3000

okonomiyaki3000 May 9, 2016

Contributor

For now, I've gone with deleting what can be deleted and displaying errors for what can't but I wrote it in such a way that it's very simple to abort before deleting anything in case of errors.

There's something else to consider though. We decide which files are good and which files are bad before firing any events. We then warn about the bad files and do nothing else with them. The good files get events fired for them at which time path to the file to delete can be changed (by a a plugin). So, a good path may become bad (because of a plugin) but a bad path has no chance to be made good.

Also, MediaControllerFile and MediaControllerFolder both have identical delete functions. One is used when deleting a single file, the other for multiple. It's kind of nonsense. I'm not going to fix it now because it's not a big deal and anyway a new media manager is being developed. Let's hope we can avoid this kind of thing in the new one.

Contributor

okonomiyaki3000 commented May 9, 2016

For now, I've gone with deleting what can be deleted and displaying errors for what can't but I wrote it in such a way that it's very simple to abort before deleting anything in case of errors.

There's something else to consider though. We decide which files are good and which files are bad before firing any events. We then warn about the bad files and do nothing else with them. The good files get events fired for them at which time path to the file to delete can be changed (by a a plugin). So, a good path may become bad (because of a plugin) but a bad path has no chance to be made good.

Also, MediaControllerFile and MediaControllerFolder both have identical delete functions. One is used when deleting a single file, the other for multiple. It's kind of nonsense. I'm not going to fix it now because it's not a big deal and anyway a new media manager is being developed. Let's hope we can avoid this kind of thing in the new one.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman May 13, 2016

Contributor

I have tested this item successfully on 3ec14a0


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

Contributor

brianteeman commented May 13, 2016

I have tested this item successfully on 3ec14a0


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

@wilsonge wilsonge removed this from the Joomla 3.6.0 milestone Jun 8, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jun 10, 2016

I have tested this item successfully on 3ec14a0


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

ghost commented Jun 10, 2016

I have tested this item successfully on 3ec14a0


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

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jun 10, 2016

Contributor

RTC


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

Contributor

brianteeman commented Jun 10, 2016

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC label Jun 10, 2016

@roland-d roland-d added this to the Joomla 3.7.0 milestone Jun 10, 2016

wilsonge added a commit that referenced this pull request Jul 30, 2016

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Jul 30, 2016

Contributor

Merged with c0aadb2. Thanks!

Contributor

wilsonge commented Jul 30, 2016

Merged with c0aadb2. Thanks!

@wilsonge wilsonge closed this Jul 30, 2016

@joomla-cms-bot joomla-cms-bot removed the RTC label Jul 30, 2016

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jul 30, 2016

Contributor

Changed milestone as it was merged early

Contributor

brianteeman commented Jul 30, 2016

Changed milestone as it was merged early

@wilsonge wilsonge modified the milestones: Joomla 3.7.0, Joomla 3.6.1 Jul 30, 2016

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Jul 30, 2016

Contributor

3.7.x branch :)

Contributor

wilsonge commented Jul 30, 2016

3.7.x branch :)

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jul 30, 2016

Contributor

Oops my mistake. Github doesn't show the bramch

Contributor

brianteeman commented Jul 30, 2016

Oops my mistake. Github doesn't show the bramch

@brianteeman brianteeman referenced this pull request Mar 29, 2017

Closed

few files to remove #13519

@okonomiyaki3000 okonomiyaki3000 deleted the okonomiyaki3000:media-select-all branch May 29, 2017

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