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 and remove tags from multiple files at once #18199

Conversation

rolandinus
Copy link

#10674
Adds an option to the actions when multiple files are selected.
The tags from all selected files are fetched and the tags which are common to all files will be preselected.
When Tags are deselected they will be removed from all selected files.
When new Tags are selected or created they will be added to all files

@rolandinus rolandinus force-pushed the feature/tag_multiple_files_at_once branch from 8f6b06a to bacfd68 Compare December 3, 2019 09:22
@kesselb kesselb added 3. to review Waiting for reviews enhancement labels Dec 3, 2019
@kesselb kesselb added this to the Nextcloud 19 milestone Dec 3, 2019
@kesselb
Copy link
Contributor

kesselb commented Dec 3, 2019

Could you rebase the branch? Should fix the test failures.

@skjnldsv skjnldsv requested a review from danxuliu December 3, 2019 13:46
@skjnldsv
Copy link
Member

skjnldsv commented Dec 3, 2019

@danxuliu could you give a hand on tests here? :)

@juliusknorr

This comment has been minimized.

@skjnldsv
Copy link
Member

skjnldsv commented Dec 5, 2019

The rebase doesn't seems right 🙈

@imtbl imtbl force-pushed the feature/tag_multiple_files_at_once branch from 1c0a7fd to 2f427c9 Compare December 5, 2019 12:55
@rolandinus rolandinus force-pushed the feature/tag_multiple_files_at_once branch from 2f427c9 to f9b0d24 Compare December 5, 2019 13:03
@rolandinus
Copy link
Author

There was some mess with the rebase, should be ok now

@skjnldsv
Copy link
Member

skjnldsv commented Dec 5, 2019

ALL GREEN!!!! @nextcloud/javascript can you believe it :D

@rolandinus rolandinus force-pushed the feature/tag_multiple_files_at_once branch 4 times, most recently from c8e4a50 to 12ed3a3 Compare December 9, 2019 08:42
@skjnldsv
Copy link
Member

Hey @rolandinus
Now that 18 is released, could you rebase your work to latest master please? :)

@rolandinus rolandinus force-pushed the feature/tag_multiple_files_at_once branch from 12ed3a3 to 1fe8d5d Compare January 28, 2020 10:52
@rolandinus
Copy link
Author

Hey @rolandinus
Now that 18 is released, could you rebase your work to latest master please? :)

I rebased it to newest master, but now I get again errors on some the tests (for server side parts which I did not touch at all) Last time they vanished after rebasing them to the newest master. But I pushed directly after rebasing. Can anyone give me information about why these errors occur and how i can avoid them?

@skjnldsv
Copy link
Member

Can anyone give me information about why these errors occur and how i can avoid them?

They're coming from anbother issue introduced in master.
They don't seems to be related to your (awesome) work :)

apps/files/js/filelist.js Outdated Show resolved Hide resolved
@rolandinus rolandinus force-pushed the feature/tag_multiple_files_at_once branch from 159c48c to 6bc92da Compare January 30, 2020 16:04
@rullzer rullzer mentioned this pull request Apr 4, 2020
80 tasks
@rullzer
Copy link
Member

rullzer commented Apr 30, 2020

I rebased this for CI

@jancborchardt
Copy link
Member

this legacy jquery style is more than just outdated. But that is the state of the files app right now, so I guess this is ok?

Yep – it is indeed outdated but that’s where we are. It’s good for 19 and a big help for people who use tagging.

Short of bigger design feedback I only have 1 point @rolandinus

  • The "Tags" action should not be the last action in the menu. Better between "Download" and "Select file range". (The last action should always be the destructive action, in this case being "Delete".

Comment on lines +105 to +109
{ name: 'tags',
displayName: 'Tags',
iconClass: 'icon-tag'

}
Copy link
Member

Choose a reason for hiding this comment

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

This block should go a bit further up so it’s below the "Download" action instead. :) Otherwise all good

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

See comment ^

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Yep – it is indeed outdated but that’s where we are. It’s good for 19 and a big help for people who use tagging.

No, sorry, I have to block.
This is too sensitive. Sorry @rolandinus , this is some areas of the code where it alwyas blow up later on. I refuse to be the one that have to fix 3 legacy files that are unrelated for this to get in.
Beside, 19 is feature freeze, nothing like that gets in for now :)

@ChristophWurst
Copy link
Member

Those who eagerly want this feature can just apply as as patch. You can download that from https://patch-diff.githubusercontent.com/raw/nextcloud/server/pull/18199.patch.

@MorrisJobke
Copy link
Member

No, sorry, I have to block.
This is too sensitive. Sorry @rolandinus , this is some areas of the code where it alwyas blow up later on. I refuse to be the one that have to fix 3 legacy files that are unrelated for this to get in.
Beside, 19 is feature freeze, nothing like that gets in for now :)

@rolandinus Unfortunately I have to acknowledge this. We are already in beta 7 and it is a too high risk of breakage here. I will move it to the 20 milestone.

@MorrisJobke MorrisJobke modified the milestones: Nextcloud 19, Nextcloud 20 May 4, 2020
@rolandinus
Copy link
Author

I know the code is outdated, I just tried to fit it in with the code of the rest of the files app. I am happy to refactor it for a future version of NC. However it would only make sense if the rest of the files app is refactored before I guess

@skjnldsv
Copy link
Member

skjnldsv commented May 4, 2020

@rolandinus Also, I want to give you some explanation, because I think you deserve to have some.
Our files app code is very old. While we're doing a crazy amount of work to have a proper implementation of our features in our apps, this is the core of our project and unfortunately it is incredibly messy. This was written in a different time and to be fair, none of us understand the code architecture. This is patched everywhere, contains lots of jQuery code, etc etc.
This makes it very hard to do anything, because any little enhancement always end up in multiple unknown bugs that takes a few days to debug.

It is never worth it.
We have plans to upgrade the files app too, but those things takes time, as youc an imagine. Keep in mind, we really are all in your awesome feature, but I'm too cold feet and cautious to approve this. For me it's not worth the risk (and we can gamble, but it always blow up somewhere unexpected, alway :( )

Hope you can understand why it takes so long for this to get approved.
Thanks again for your work and interest! 🤗 🙇‍♀️

@TueTchen
Copy link

TueTchen commented May 4, 2020

Thank you all for your efforts and your explanations!
In the meantime I'll help myself/ourselves by using the good old school tool 'nc-direct' (= SQL 😅). The tag relation are really very simple, and our acribic file naming help us to get a main mass tagging done on its lowest level.
Regarding our wider usage of tagging (currently >100 tags for >3000 files), there are also growing new requirement and wishes, that underline a well-thought refactoring - e.g. hierarchical tag relations, access rights enhancements and also another kind of showing and handling file taggings (we've already up to 20 tags for the single file, it's not really easy to identify missing or wrong tags).

So thank you again, always good luck & good speed and please stay healthy!
Peter

@MorrisJobke MorrisJobke mentioned this pull request Aug 11, 2020
57 tasks
This was referenced Dec 14, 2020
This was referenced Dec 28, 2020
@rullzer rullzer modified the milestones: Nextcloud 21, Nextcloud 22 Jan 6, 2021
@MorrisJobke MorrisJobke self-assigned this Mar 17, 2021
@HTStentiQ

This comment has been minimized.

@skjnldsv skjnldsv closed this Apr 1, 2021
@skjnldsv
Copy link
Member

skjnldsv commented Apr 1, 2021

Closing, see #18199 (comment)

@MorrisJobke MorrisJobke removed their assignment Apr 1, 2021
@Sandro1955
Copy link

Hi, I installed version 12.01, but I can't find the "multiple tags" function in the actions. How do I go about implementing it?

@Sandro1955
Copy link

Hi, I installed version 21.0.4, but I can't find the "multiple tags" function in the actions. How do I go about implementing it?

@skjnldsv
Copy link
Member

skjnldsv commented Aug 11, 2021

#28133 & #28135

@nextcloud nextcloud locked as resolved and limited conversation to collaborators Aug 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.