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

Upgrade sidebar search #614

Merged
merged 28 commits into from
Feb 20, 2025
Merged

Conversation

BhagatHarsh
Copy link
Contributor

@BhagatHarsh BhagatHarsh commented Feb 12, 2025

  • Addresses: Search Bar Support in Side Bar #573
  • Code improvements in handing sidebar movements
  • Code improvements in rendering and operations related to pinned and disk divider in sidebar directories
  • Improved render and used pre-rendered strings for efficiency

Copy link

netlify bot commented Feb 12, 2025

Deploy Preview for superfile canceled.

Name Link
🔨 Latest commit 9879842
🔍 Latest deploy log https://app.netlify.com/sites/superfile/deploys/67b6e9eca10a8c0008ce118f

@BhagatHarsh BhagatHarsh marked this pull request as draft February 13, 2025 04:51
@BhagatHarsh BhagatHarsh marked this pull request as ready for review February 14, 2025 13:24
@lazysegtree
Copy link
Collaborator

@BhagatHarsh We have fuzzy search implemented in file panel search. It would be inconsistent if sidebar search is not fuzzy search
For example mts and ments both should be able to find Documents

image
image image

@lazysegtree
Copy link
Collaborator

lazysegtree commented Feb 14, 2025

Also make sure to rebase your branch against main . Some new commits have went in recently.
And make sure to have code formatted as per go fmt Thats why, the go build check is failing

Just run go fmt ./... in project root, and go will fix the code formatting.

@lazysegtree
Copy link
Collaborator

lazysegtree commented Feb 14, 2025

@BhagatHarsh It would be great to keep the same sidebar grouping also on search results

image image
Sample changes

func getFilteredDirectories(query string) []directory {
	directories := []directory{}

	directories = append(directories, 
		getFilteredDirectoriesFromSource(query, getWellKnownDirectories())...)
	directories = append(directories, directory{
		// Just make sure no one owns the hard drive or directory named this path
		location: "Pinned+-*/=?",
	})
	directories = append(directories, 
		getFilteredDirectoriesFromSource(query, getPinnedDirectories())...)
	directories = append(directories, directory{
		// Just make sure no one owns the hard drive or directory named this path
		location: "Disks+-*/=?",
	})
	directories = append(directories, 
		getFilteredDirectoriesFromSource(query, getExternalMediaFolders())...)
	return directories
	

}
func getFilteredDirectoriesFromSource(query string, source []directory) []directory {
	query = strings.ToLower(query)
	if query == "" {
		return source
	}

	var filteredDirectories []directory
	for _, dir := range source {
		if strings.Contains(strings.ToLower(dir.name), query) {
			filteredDirectories = append(filteredDirectories, dir)
		}
	}
	return filteredDirectories
}

@lazysegtree

This comment was marked as outdated.

@BhagatHarsh

This comment was marked as outdated.

@lazysegtree

This comment was marked as outdated.

@lazysegtree
Copy link
Collaborator

@BhagatHarsh maybe dont spend time on fixing any comments for now. Let me send a patch.

@BhagatHarsh BhagatHarsh force-pushed the upgrade_sidebar_search branch from eb49be9 to c69b2c4 Compare February 16, 2025 05:37
@BhagatHarsh
Copy link
Contributor Author

BhagatHarsh commented Feb 16, 2025

Some issues identified in testing

  • Rename feature after search does not work
  • If a sidebar search leads to a directory in which the cursor was not present then trying to directly go to that directory crashes the app.
  • after search the cursor may disappear (need to rerender from top)
  • when during the search no results are found, then we must display [None]

@lazysegtree lazysegtree force-pushed the upgrade_sidebar_search branch from 459188b to 6bc2257 Compare February 17, 2025 13:33
lazysegtree
lazysegtree previously approved these changes Feb 17, 2025
@@ -404,135 +410,91 @@ func (m *model) itemSelectDown(wheel bool) {

// ======================================== Sidebar controller ========================================

// Yorukot: P.S God bless me, this sidebar controller code is really ugly...
Copy link
Collaborator

Choose a reason for hiding this comment

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

@yorukot Please check the current code. I have improved it now.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes it's really great!!

// Otherwise dividers are sometimes skipped in render in case of
// large pinned directories
s.updateRenderIndex(mainPanelHeight)
if s.directories[s.cursor].isDivider() {
Copy link
Collaborator

@lazysegtree lazysegtree Feb 17, 2025

Choose a reason for hiding this comment

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

Since s.noActualDir() is not true. There definitely are non-Divider normar directories, and code will never get stuck in infinite loop

package internal

// These are effectively consts
var pinnedDividerDir = directory{
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have used a sentinel value, and we will use it everywhere instead of using "Pinned+-*/=?" string constant everywhere

@lazysegtree lazysegtree requested a review from yorukot February 17, 2025 14:04
location: "Disks+-*/=?",
})
directories = append(directories, getExternalMediaFolders()...)
func formDirctorySlice(homeDirectories []directory, pinnedDirectories []directory, diskDirectories []directory) []directory {

Choose a reason for hiding this comment

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

is this checking that no one owns the hard drive or directory ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but why do we need that?

Choose a reason for hiding this comment

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

im assuming the original comment is talking about directories belonging to other users or os's , we dont want to display them as a normal directory, theyll need to be specifically displayed as not owned by current user.

@lazysegtree your thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me check this today. I have been wanting to test and understand the disk's part of sidebar, but my mac has no disks. I will try to set something up

Copy link
Collaborator

Choose a reason for hiding this comment

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

@anaypurohit0907 external directories are displayed seperately.
Although we dont do any specific ownership check, we just utilize disk.Partitions from github.com/shirou/gopsutil/disk package, and get disk partitions. Additional partitions other then the current partitions of superfile are listed in disks section. Since they are displayed under disk's, we dont need to explicitly tell that "not owned by current user"

Screenshot 2025-02-20 at 7 36 49 AM

@yorukot
Copy link
Owner

yorukot commented Feb 19, 2025

Overall I think it's very good,Thanks for you guys! but I think the search bar should be hidden when not in use.

Of course, if you think this improvement is not good, please tell me, I am willing to discuss!

cc: @BhagatHarsh @lazysegtree

yorukot
yorukot previously approved these changes Feb 19, 2025
@BhagatHarsh
Copy link
Contributor Author

Overall I think it's very good,Thanks for you guys! but I think the search bar should be hidden when not in use.

But then how will the user know that this feature exists?

Copy link
Owner

@yorukot yorukot left a comment

Choose a reason for hiding this comment

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

Sorry my bad :/ I updated to the wrong branch.

@BhagatHarsh BhagatHarsh dismissed stale reviews from yorukot and lazysegtree via 9879842 February 20, 2025 09:43
Copy link
Collaborator

@lazysegtree lazysegtree left a comment

Choose a reason for hiding this comment

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

Looks good now. I think we are okay to merge

@yorukot yorukot merged commit 0cd46c4 into yorukot:main Feb 20, 2025
6 checks passed
@BhagatHarsh BhagatHarsh deleted the upgrade_sidebar_search branch February 22, 2025 03:35
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.

4 participants