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

Added filter by filesystem location + bonus tooltip on overflowed filter names #518

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

FinalDoom
Copy link
Contributor

New filter section that allows viewing space usage/torrent count by directory in tree format.
Filter names overflow with ellipses, and in the case of root directories, this is common: so I added a title-based tooltip to filter elements where the text is overflowed. Only appears on overflowed elements, and on mouse event. Not sure if equivalent touch events would work the same way.

No issue submitted, just something I wanted to make.

image
image

  • Breaking change (changes that break backward compatibility of public API or CLI - semver MAJOR)
  • New feature (non-breaking change which adds functionality - semver MINOR)
  • Bug fix (non-breaking change which fixes an issue - semver PATCH)

@codecov
Copy link

codecov bot commented Feb 20, 2022

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (1510940) 72.94% compared to head (916f8de) 73.23%.

Files Patch % Lines
server/services/taxonomyService.ts 97.67% 1 Missing ⚠️
server/util/fileUtil.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #518      +/-   ##
==========================================
+ Coverage   72.94%   73.23%   +0.28%     
==========================================
  Files          62       62              
  Lines       11375    11409      +34     
  Branches      951      969      +18     
==========================================
+ Hits         8298     8355      +57     
+ Misses       3063     3040      -23     
  Partials       14       14              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mouzzampk2014
Copy link

This is brilliant, thanks for working on this.

@jesec
Copy link
Owner

jesec commented Feb 24, 2022

The general idea looks OK, but the operations could get prohibitively expensive when there are many entries.

@FinalDoom
Copy link
Contributor Author

FinalDoom commented Feb 24, 2022

Open to other algorithms, but I can attest it takes minimal amount of time more than master with ~6k torrents.
image
And with 10^5 directories (actual insanity IMO); 10 directories each with 10 children, each with 10 children, and so on, 5 deep.
image

Would it be worth making a default-disabled setting to enable the feature?

@jesec
Copy link
Owner

jesec commented Feb 25, 2022

Open to other algorithms, but I can attest it takes minimal amount of time more than master with ~6k torrents. image And with 10^5 directories (actual insanity IMO); 10 directories each with 10 children, each with 10 children, and so on, 5 deep. image

Numbers look acceptable, but I would assume there is a non-trivial amount of associated CPU usage. I am reluctant to add a potentially expensive operation to the poll routine. We can probably employ more memoization.

Would it be worth making a default-disabled setting to enable the feature?

Not a good idea when the server-side functions are involved. There could be API concerns.

I am tied up with some other matters at hand, and I will take a deeper look next week.

@boeto
Copy link

boeto commented Nov 12, 2022

When will it be merge? Expect this awesome feature

@trim21
Copy link
Collaborator

trim21 commented Dec 17, 2023

can you resolve conflicts?

@FinalDoom
Copy link
Contributor Author

I can, are you looking to actually merge it? (don't wanna play the fix/wait/fix/wait game)

@trim21
Copy link
Collaborator

trim21 commented Dec 20, 2023

I can, are you looking to actually merge it? (don't wanna play the fix/wait/fix/wait game)

yes

@FinalDoom
Copy link
Contributor Author

Should be good now, also changed the mouseover to a hook, should be nicer.

@trim21
Copy link
Collaborator

trim21 commented Dec 21, 2023

give me some time to test this locally

@trim21
Copy link
Collaborator

trim21 commented Dec 21, 2023

this doesn't look right, in my understand shouldn't this display like a tree or multiple trees? but some non-root elements are displayed at top level

image

image
image

download dir here are /srv/dev-disk-by-*/.../... and /export/3t-1/...

@trim21
Copy link
Collaborator

trim21 commented Dec 21, 2023

also, just a question, is it possible to build trees at client instead of server like tags?

@FinalDoom
Copy link
Contributor Author

shouldn't this display like a tree or multiple trees?

I'll have to take a look at it again, as it's been a while; but iirc the trees are built by comparing common paths, removing prefixes, and traversing down. I think the paths come from those reported on the torrent metadata.. so is it possible those torrents are using relative paths or something starting with the hex?

Which root path should those non-root elements be under?

is it possible to build trees at client instead of server like tags?

Probably? I'm guessing I mimicked something that needed to be server-side, but again it's been a while. I'll have to take a look at what metadata ends up on the client.

@trim21
Copy link
Collaborator

trim21 commented Dec 22, 2023

so is it possible those torrents are using relative paths or something starting with the hex?

Which root path should those non-root elements be under?

no, they are all abs path.

My torrents' download directories:

  • /srv/dev-disk-by-uuid-58b9efe4-428e-40d1-8d3f-97d8a5bac12a/u2-small/{hex}
  • /srv/dev-disk-by-uuid-3c509d6a-504e-4d71-a19d-5f81c62d9902/...
  • /srv/dev-disk-by-uuid-41a30911-024f-4bd6-bb21-2aa90bed52e9/...
  • /srv/dev-disk-by-uuid-58b9efe4-428e-40d1-8d3f-97d8a5bac12a/...
  • /export/3t-1/downloads/...

so all hex elements and elements starts by dev-disk-by-uuid should be non-root elements, only /srv/ and /export/3t-1/downloads should be root element.

@FinalDoom
Copy link
Contributor Author

Very odd. I'll have to take another look and make some test cases when I get a chance.

@trim21
Copy link
Collaborator

trim21 commented Dec 23, 2023

looks like I have mixed case directories and they cause this bug:

  • /srv/dev-disk-by-uuid-58b9efe4-428e-40d1-8d3f-97d8a5bac12a/U2-small/e9b3a6c75170e0c14701e7095b94412cfd4c7c7d
  • /srv/dev-disk-by-uuid-58b9efe4-428e-40d1-8d3f-97d8a5bac12a/u2-small/00ac7be5eb069dd8af992fca3a40b3755aaad846

@trim21
Copy link
Collaborator

trim21 commented Dec 23, 2023

console.log(
  [
    '/srv/dev-disk-by-uuid-58b9efe4-428e-40d1-8d3f-97d8a5bac12a/u2-small/231d4f952201ba96628a9520740554c6ae6837db',
    '/srv/dev-disk-by-uuid-58b9efe4-428e-40d1-8d3f-97d8a5bac12a/U2-small/232e75cf79187a07a7507f82275f1e8d3f7ddf73',
    '/srv/dev-disk-by-uuid-58b9efe4-428e-40d1-8d3f-97d8a5bac12a/u2-small/2364ebde4155eaf3267af1011b4dbc9cc75d99e1',
  ].sort((a, b) => {
    return a.localeCompare(b);
  }),
);

=>

[
  '/srv/dev-disk-by-uuid-58b9efe4-428e-40d1-8d3f-97d8a5bac12a/u2-small/231d4f952201ba96628a9520740554c6ae6837db',
  '/srv/dev-disk-by-uuid-58b9efe4-428e-40d1-8d3f-97d8a5bac12a/U2-small/232e75cf79187a07a7507f82275f1e8d3f7ddf73',
  '/srv/dev-disk-by-uuid-58b9efe4-428e-40d1-8d3f-97d8a5bac12a/u2-small/2364ebde4155eaf3267af1011b4dbc9cc75d99e1'
]

@trim21
Copy link
Collaborator

trim21 commented Dec 24, 2023

lgtm, only one minor problem

@FinalDoom
Copy link
Contributor Author

I have a reduced solution that can probably be moved client-side; but case-insensitivity causes problems when filtering torrents. I think the solution is for the tree to be client-side, and the server can tell the client if the OS it's running on is case-insensitive or not, allowing insensitive tree and filtering

The current solution means client has no clue on insensitivity, so filtering on non-matching lowercased names omits torrents incorrectly.

Another option would be to always treat everything as case sensitive, and you'd end up with duplicate paths where casing doesn't match.

But it's late, so I'll give it a shot tomorrow.

@trim21
Copy link
Collaborator

trim21 commented Dec 24, 2023

I have a reduced solution that can probably be moved client-side; but case-insensitivity causes problems when filtering torrents. I think the solution is for the tree to be client-side, and the server can tell the client if the OS it's running on is case-insensitive or not, allowing insensitive tree and filtering

The current solution means client has no clue on insensitivity, so filtering on non-matching lowercased names omits torrents incorrectly.

Another option would be to always treat everything as case sensitive, and you'd end up with duplicate paths where casing doesn't match.

But it's late, so I'll give it a shot tomorrow.

after reviewing the code I think there is no need to move it to client side, current code looks fine

@trim21
Copy link
Collaborator

trim21 commented Dec 24, 2023

handling case-insensitivity may introduce too much extra complexity,which will need to handle every component of path,I think current code works fine

@FinalDoom
Copy link
Contributor Author

I looked a bit at moving the treeing to client-side:

I'm guessing there's some reason for building the tag list and other sidebar filters server-side, whether it's shared efficiency for multiple clients, the taxonomy diff data savings, or something else. I reduced the complexity of computing the tree and just stored the counts/data size in-tree, so it should be a little faster/simpler overall.

There may be an argument for moving filter value computation to the client, but I think matching the existing sidebar filter logic makes more sense for now.

Just treat everything as case-sensitive.
Also flood could be on a different OS than the torrent application, which would also cause mismatches.
@trim21
Copy link
Collaborator

trim21 commented Jan 2, 2024

please do not force push

@FinalDoom
Copy link
Contributor Author

I don't see what's failing in the tests. It names the new test file but there's no failing test or other message. It passes locally. Github doesn't have a way to rerun workflows?

@FinalDoom
Copy link
Contributor Author

please do not force push

It's in my repo and rebasing creates better changeset history, why not?

@trim21
Copy link
Collaborator

trim21 commented Jan 3, 2024

please do not force push

It's in my repo and rebasing creates better changeset history, why not?

Force-push causes inconvenient in code reviewing

return true;
}
return false;
return !!realPath?.startsWith(allowedPath);
Copy link
Collaborator

Choose a reason for hiding this comment

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

please do not touch this file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants