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

Rework media library rename functionality #509

Open
onli opened this Issue Apr 30, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@onli
Copy link
Member

onli commented Apr 30, 2017

That the existing code has grown a bit wild can already be seen by serendipity_moveMediaDirectory being the function that gets called when a file gets renamed. This issue covers the project to rework the code to work with the new responsive thumbnails (#474), be more reliable (see https://board.s9y.org/viewtopic.php?f=3&t=21185), easier readable, and calling functions that actually do what their name says.

  • Rework code to be more sound and work with responsive thumbnails
  • Re-add ACL functionality
  • Make sure the new approach works reliable (user testing, but try to also implement unit tests)
  • Also change the image link, not just the img src

@onli onli added this to the Major milestone Apr 30, 2017

@onli onli self-assigned this Apr 30, 2017

@onli

This comment has been minimized.

Copy link
Member Author

onli commented Apr 30, 2017

First commit for this: bfeccab

onli referenced this issue Apr 30, 2017

Rework media library directory move
Initial motivation for this rework was to add support for the responsive thumbnnails (#474). But it also is a re-implementation instead of an enahncement of the existing code because the moveMediaDirectory function had grown into a mess. It was very hard to debug possible renaming bugs, like https://board.s9y.org/viewtopic.php?f=3&t=21185. This approach uses several small functions instead that can be combined and re-used.
@onli

This comment has been minimized.

Copy link
Member Author

onli commented Jun 12, 2018

@garvinhicking Did you have some success with the ACL functionality?

@onli

This comment has been minimized.

Copy link
Member Author

onli commented Oct 30, 2018

Current bug: The href attribute of links that come before the s9yimdb comment are currently not replaced. Maybe re-add logic similar to the REGEXP replacements of staticpage, or use a sqlite compatible way - or use a new regex following the current scheme.

ACLs can be re-added to the api event hook call place in the code. Garvin will look at this :)

garvinhicking pushed a commit that referenced this issue Nov 5, 2018

[BUGFIX] Fixes media library regression, references #509
Adds missing ACL renames
Missing trailing / when managing dirs
Fix typo that did not evaluate read/write properly
Add missing NEWS entries
@garvinhicking

This comment has been minimized.

Copy link
Member

garvinhicking commented Nov 5, 2018

I have re-added the ACL replacements.

What's missing now but important for a release is the REGEXP replacement thing (see old code: "Entry REPLACEMENT AREA").

Also, I think currently renaming the mediaproperties database table is missing as well. This took care of replacing old TITLE, realname and name attributes of a media file. I think this will need to be re-added into the serendipity_updateImageInDatabase function, please have a look.

@onli

This comment has been minimized.

Copy link
Member Author

onli commented Nov 18, 2018

What's missing now but important for a release is the REGEXP replacement thing (see old code: "Entry REPLACEMENT AREA").

I added this in 732f1ae. The approach there has one problem: We do not know whether the old link pointed to something other than the image. Maybe you have an idea how to detect that, how to get the old image link maybe?

@onli

This comment has been minimized.

Copy link
Member Author

onli commented Nov 18, 2018

Also, I think currently renaming the mediaproperties database table is missing as well. This took care of replacing old TITLE, realname and name attributes of a media file.

Is this about

// renaming filenames has to update mediaproperties if set
? In which circumstances are those values set? I do not see it in my database nor the code, and since in the UI all seems to update now properly I'm surprised there is a second layer?

Edit: Strike that, I think I got it. When going into the image properties in the ML and clicking on "Go" it saves all that data to the database. I just pushed a commit that restores the update of those values.

@onli

This comment has been minimized.

Copy link
Member Author

onli commented Feb 10, 2019

The approach there has one problem: We do not know whether the old link pointed to something other than the image.

This is done now. In my eyes this just needs testing, which this rework would get automatically in a beta. I'll remove the blocking tag.

@onli onli removed the blocking label Feb 10, 2019

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