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

Exclude hidden files from recent recommendations #354

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blkqi
Copy link

@blkqi blkqi commented Jan 13, 2021

Resolves #71 by

  • Restructuring getMostRecentRecommendation to better allow for filtering of results.
  • Implementing filter isNodeExcluded in order to exclude nodes which are either hidden files or descendants of hidden folders. A traversal of the file system hierarchy is necessary to find hidden ancestors, which greatly reduces the worst-case time complexity.
  • Respecting the user's configuration selection regarding hidden files.

The number of candidate files is arbitrarily limited by 10 times the actual number requested.

Signed-off-by: Brett Kleinschmidt <blk@blk.me>
@blkqi blkqi force-pushed the exclude-hidden-recent-files branch from 7225783 to 80d1349 Compare January 13, 2021 01:49
try {
$next = $next->getParent();
} catch (NotFoundException $e) {
break;
Copy link
Member

@ChristophWurst ChristophWurst Jan 13, 2021

Choose a reason for hiding this comment

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

won't you run into an infinite loop here?

I think you would be better off with recursion. If the current node is hidden, you return false. If there is a parent you return $this->isNodeExcluded($node->getParent(), $showHidden);. Else you return false.

Copy link
Author

@blkqi blkqi Jan 13, 2021

Choose a reason for hiding this comment

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

Thank you for taking a look!

The sequence ends when a dotfile is found (return true) or the root node is found (break). In other words, it is always bounded by the depth of the node. If there were a clearer way to identify the root node other than by exception handling then I would be more than happy to use it.

While I agree that recursion is generally a more natural approach to tree traversal problems, I don't think anything is lost in this context by iterating. And in the recursive case, we would want to introduce a new function so that isNodeExcluded can be adapted to other types of exclusions related to the target node (name, mimetype, tag, etc.).

@blkqi blkqi requested a review from ChristophWurst January 18, 2021 23:08
@blizzz blizzz added this to the Nextcloud 25 milestone Apr 21, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@skjnldsv skjnldsv removed this from the Nextcloud 29 milestone Feb 8, 2024
@skjnldsv
Copy link
Member

skjnldsv commented Feb 8, 2024

Please rebase? 🙈

@skjnldsv skjnldsv added enhancement New feature or request 2. developing labels Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show only "visible" recommendations
4 participants