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

Rename "albums" to "folders" #457

Merged
merged 2 commits into from
Oct 15, 2020
Merged

Rename "albums" to "folders" #457

merged 2 commits into from
Oct 15, 2020

Conversation

strugee
Copy link
Member

@strugee strugee commented Sep 22, 2020

This more accurately reflects what the feature actually is, and paves the way for the addition of a "real" virtual albums feature as discussed in #120.

This is my first significant contribution to any Nextcloud app, so when reviewing, please do not assume I know what I'm doing :P

@strugee strugee requested a review from skjnldsv September 22, 2020 02:51
@strugee strugee force-pushed the strugee/albums-to-folders branch 2 times, most recently from 3abe104 to 808c889 Compare September 22, 2020 03:22
@strugee
Copy link
Member Author

strugee commented Sep 22, 2020

CI is failing because the Webpack build leaves the working tree dirty. I'll push an amended patchset that includes the changes to them, since AFAICT that's what I'm supposed to do? The contributing docs don't mention it (I'm just guessing based on the nextcloud/server README).

@strugee strugee force-pushed the strugee/albums-to-folders branch from 808c889 to 9745113 Compare September 22, 2020 06:08
@strugee
Copy link
Member Author

strugee commented Sep 23, 2020

Oh, another note: I did not delete localization strings for "Your album" under the assumption that they will be needed soon. (Indeed locally I'm working through adding a virtual albums feature that would use them again.)

@skjnldsv skjnldsv added 3. to review Waiting for reviews enhancement New feature or request labels Sep 30, 2020
@jancborchardt
Copy link
Member

Sorry for the late reply @strugee!

I’m not entirely sure about this. We should not have both "Folders" and "Albums" as that will create confusion. Every folder should just be an album (as now, for fallback) and then the new possible Albums implementation should also allow virtual albums.

Or what do you think @skjnldsv @karlitschek @jakobroehrl @Mikescops @dassio @nextcloud/photos – since it’s more than just a wording change.

@da-anda
Copy link

da-anda commented Oct 7, 2020

since you can not configure what is scanned/displayed in the photos app as "albums", I very much support that the current implementation is renamed to just "folders". The cover art of my music collection certainly is not an album and thus should never ever show up as such. Folders are folders and albums are a virtual collection of files from a single or multiple folders.

@Mikescops
Copy link
Member

To be honest @jancborchardt I'm fine with the Folders rename, it's the truth we just browse the file directory.

We shouldn't rely on what things could be in the future because as we know future features can be really far from now. And the current "Album" naming is indeed misleading.

@karlitschek
Copy link
Member

Sorry for the late reply @strugee!

I’m not entirely sure about this. We should not have both "Folders" and "Albums" as that will create confusion. Every folder should just be an album (as now, for fallback) and then the new possible Albums implementation should also allow virtual albums.

Or what do you think @skjnldsv @karlitschek @jakobroehrl @Mikescops @dassio @nextcloud/photos – since it’s more than just a wording change.

No strong opinions here to be honest.

@Mikescops
Copy link
Member

Can we move on here?

@strugee
Copy link
Member Author

strugee commented Oct 13, 2020

Meant to reply to this and forgot, sorry. I do like @jancborchardt's proposed design (we could call folders "Folder Albums" or something and have an option to hide them, for cases like @da-anda's music cover art) but realistically that feature might be a ways off.

I propose we merge this as-is since it more accurately reflects the current feature set. Whoever implements real virtual albums can just revert the name change. If that sounds agreeable to everybody I'll go ahead and rebase this.

@da-anda
Copy link

da-anda commented Oct 13, 2020

In order to be more clear that the nodes do only list folders containing pictures, I think the shown headlines in the my folders and my shared folders nodes could be a bit clearer about this and say something like Your picture folders, Folders containing pictures or something like that.

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.

Let’s go with the rename then since it indeed reflects the current state. :) We can always adjust later on.

(Just don’t want the navigation to become filled with different terms of "Album" and "Folder" in the long run, to make that clear.)

@skjnldsv
Copy link
Member

skjnldsv commented Oct 13, 2020

@strugee please rebase? (you can drop changes for /js) :)

@strugee
Copy link
Member Author

strugee commented Oct 13, 2020

@skjnldsv what do you mean drop changes for js/? The original patchset in this PR didn't include changes in js/, but CI complained about that so I added them. Won't that happen again?

@da-anda I'm on the fence; I see what you're saying but I think those suggestions would be too wordy for the UI. And it's the Photos app anyway, so I think there's probably enough context for the user.

@skjnldsv
Copy link
Member

@skjnldsv what do you mean drop changes for js/? The original patchset in this PR didn't include changes in js/, but CI complained about that so I added them. Won't that happen again?

For the rebase, you can ignore all conflicts for js files as you will have to recompile them afterwards :)

@strugee strugee force-pushed the strugee/albums-to-folders branch from 9745113 to e661d03 Compare October 13, 2020 19:53
@strugee
Copy link
Member Author

strugee commented Oct 13, 2020

This should be ready for merge.

Ref #120

Signed-off-by: AJ Jordan <alex@strugee.net>
@skjnldsv skjnldsv force-pushed the strugee/albums-to-folders branch from e661d03 to b8292b6 Compare October 14, 2020 16:37
@skjnldsv
Copy link
Member

/compile amend /

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 14, 2020
Ref #120

Signed-off-by: AJ Jordan <alex@strugee.net>
Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
@npmbuildbot-nextcloud npmbuildbot-nextcloud bot force-pushed the strugee/albums-to-folders branch from b8292b6 to 3f40366 Compare October 14, 2020 16:40
@skjnldsv skjnldsv merged commit c3b471c into master Oct 15, 2020
@skjnldsv skjnldsv deleted the strugee/albums-to-folders branch October 15, 2020 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants