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 opening federated shares #50292

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

provokateurin
Copy link
Member

Summary

Opening a federated share is currently broken, as the scroll does not work for items that are in a certain area outside the viewport and the viewer is not triggered because the permissions are wrong.

I tested this on stable30 and stable29 as well and those have the same bug and the fixes also work there.

Checklist

@provokateurin provokateurin added bug 3. to review Waiting for reviews labels Jan 21, 2025
@provokateurin provokateurin added this to the Nextcloud 31 milestone Jan 21, 2025
@provokateurin provokateurin requested a review from susnux January 21, 2025 14:10
@provokateurin provokateurin force-pushed the fix/federated-share-opening branch from 24a89e5 to e485f58 Compare January 21, 2025 14:31
@@ -296,7 +296,8 @@ export default defineComponent({

// Check if the content is smaller than the viewport, meaning no scrollbar
const targetRow = Math.ceil(this.dataSources.length / this.columnCount)
if (targetRow < this.rowCount) {
// The offset is needed to avoid some items not being visible but also not being scrolled to.
if (targetRow < this.rowCount - 8) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sure it is not this? (8 is such a magic number)

Suggested change
if (targetRow < this.rowCount - 8) {
if (targetRow < (this.rowCount - ((this.bufferItems / this.columnCount) * 2)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I hate this magic number. I just checked which value fits and 8 is the one. I'm not sure on what it is based, but I can try your calculation.

Copy link
Member Author

@provokateurin provokateurin Jan 21, 2025

Choose a reason for hiding this comment

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

So it's 6 in list mode, but that doesn't work if the item is just one off screen. 8 is the minimum value that works here.

@Altahrim Altahrim mentioned this pull request Jan 21, 2025
@provokateurin
Copy link
Member Author

I noticed the viewer still fails to open when you do the process multiple times in a row. The first time works with this PR, but the subsequent attempts still fail.

@provokateurin provokateurin marked this pull request as draft January 21, 2025 16:28
@provokateurin provokateurin added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants