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

Fix fetching album content when navigating #1388

Merged
merged 1 commit into from
Oct 20, 2022

Conversation

artonge
Copy link
Collaborator

@artonge artonge commented Oct 19, 2022

When navigating between albums:

  • The call to newFileMenuEntry was causing an error, because the entry were already existing as we did not remove it when destroying AlbumContent
  • fetchAlbumContent was never called as the album's fileName did not change
  • addFilesToAlbum was wrongly incrementing the items count, so I created another mutation called setAlbumFiles that only set the album's files.

@artonge artonge added bug Something isn't working 3. to review Waiting for reviews labels Oct 19, 2022
@artonge artonge added this to the Nextcloud 26 milestone Oct 19, 2022
@artonge artonge requested a review from skjnldsv October 19, 2022 14:40
@artonge artonge self-assigned this Oct 19, 2022
@artonge
Copy link
Collaborator Author

artonge commented Oct 19, 2022

/backport to stable25

@backportbot-nextcloud backportbot-nextcloud bot added the backport-request Pending backport by the backport-bot label Oct 19, 2022
@artonge artonge force-pushed the artonge/fix/fetching_album_content branch 2 times, most recently from a9cca46 to 3bb10dc Compare October 19, 2022 15:17
...state.albumsFiles,
[albumName]: [
...albumFiles,
...fileIds.filter(fileId => !albumFiles.includes(fileId)), // Filter to prevent duplicate fileId.
Copy link
Member

Choose a reason for hiding this comment

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

We should really prefer using Set for performances reasons :)
https://jsben.ch/JNnpx

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can we address that later during a refactoring ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But you made your point :)

Copy link
Collaborator Author

@artonge artonge Oct 19, 2022

Choose a reason for hiding this comment

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

I actually already made the change in collectionStoreFactory that I want to use for albums and share albums

[`addFilesTo${capitalizedCollectionName}`](state, { collectionId, fileIdsToAdd }) {
const collectionFiles = state[`${collectionName}sFiles`][collectionId] || []
state[`${collectionName}sFiles`] = {
...state[`${collectionName}sFiles`],
[collectionId]: [...new Set([...collectionFiles, ...fileIdsToAdd])],
}
state[`${collectionName}s`][collectionId].nbItems += fileIdsToAdd.length
},

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But truth for truth, I am actually thinking of more generic store structure instead of having dedicated stores with collectionStoreFactory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But first, make it work, then optimize

Copy link
Member

Choose a reason for hiding this comment

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

Can we address that later during a refactoring ?

Absolutely 🎉

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if there isn't some tools out there that can detect such things 🤔
That seems not too crazy. Easy recommendations tbh 🙈

Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge force-pushed the artonge/fix/fetching_album_content branch from 3bb10dc to e739e51 Compare October 20, 2022 10:18
@artonge artonge enabled auto-merge October 20, 2022 10:18
@artonge artonge merged commit f747b47 into master Oct 20, 2022
@artonge artonge deleted the artonge/fix/fetching_album_content branch October 20, 2022 11:29
@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

@artonge
Copy link
Collaborator Author

artonge commented Oct 20, 2022

/backport to stable25

@backportbot-nextcloud backportbot-nextcloud bot removed the backport-request Pending backport by the backport-bot label Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants