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

Feature/quick search #66

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

Conversation

iksi4prs
Copy link
Contributor

Filters out item as you time.
Can be controlled via settings
as was requested here - #59

image

* 1st version of adding system icon for files

* added SystemIconsService

* continued implementation

* changed reminder tag

* added support for extension who use uwp apps

* added support also to indexing using ,

* added wip

* added support to resolve lnk files

* fixed some lnk issues

* moved some code to new service IShellLinksService

* added fallback to builtin icons, when there is no shell icon

* updated comments

* renamed enum value

* added ShellIconsCacheService

* implemented cache

* added todos

* added ImageModel and csproj to implement it

* cleanup

* moved System.Drawing.Common to windows project

* started adding filter

* started adding settings for icons

* added comment

* some imporvments

* fix

* added caching of value

* added some notes in settings dialog

* renamed IconsService to IconsSettingsService

* continued rename

* added check of Windows

* cleanup

* cleanup

* renamed 'SystemIcon' to 'ShellIcon'

* moved strings to resources

* fixed comment

* cleanup and made code more clear

* changed code to be more clear

* cleanup

* fixed - dont crash on linux

* some cleanup and comments

* added key down on filters also selects next item

* moved code to new service 'quick-search

* added stubs for TextInput event

* cleanup

* add usage of TextInput instead of KeyToChar

* added settings dialogs

* renamed folder

* renamed namespace

* renamed file

* renamed interface

* renmaed enum

* renamed member

* renamed arg

* renamed file

* renamed class

* added comment

* fixed text on label

* added explenation

* fixed comment

* fixed todo

* added todo

* removed comment

* updated comments

* updated comment

* fixed: make sure to scroll to item

* refactoring in GetSelected

* fixed going to next row, using same letter repeatedly

* refactoring and fix of select of first item

* also reset selected index on clear of search

* cleanup

* fixed icon of keyboard in settings (before was copy of some existing icon)

* added help and fixed look of settings, almost no need for groupbox now

* added binding to disable row

* cleanup

* cleanup from unused style classes

* also go back when shift pressed

* cleanup

* moved code to static function so will be more clear

* some fixes

* more fix

* added clear search when switcing folders

* added comment

* added comment

* added comment and cleanup

* fixed all build error after sync from fork

* removed duplicate of file which was moved to other folder

* cleanup

* cleanup 2

* cleanup and comments

* added comments

* removed un-needed file

* added comments + cleanup

* cleanup 3

* added comments + cleanup 2

* remove unused file

* added comment

* cleanup
@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2023

Codecov Report

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

Comparison is base (3330b67) 92.89% compared to head (14abb1d) 91.42%.

Files Patch % Lines
...tions/MainWindow/FilePanels/FilesPanelViewModel.cs 31.81% 27 Missing and 3 partials ⚠️
...ions/MainWindow/FilePanels/QuickSearchViewModel.cs 47.36% 19 Missing and 1 partial ⚠️
...lementations/Settings/KeyboardSettingsViewModel.cs 20.00% 16 Missing ⚠️
src/Camelot.Services/QuickSearchService.cs 57.89% 8 Missing ⚠️
...ifications/QuickSearch/QuickSearchSpecification.cs 0.00% 6 Missing ⚠️
...s/MainWindow/FilePanels/QuickSearchCommandModel.cs 0.00% 6 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
- Coverage   92.89%   91.42%   -1.48%     
==========================================
  Files         185      192       +7     
  Lines        4830     4967     +137     
  Branches      388      403      +15     
==========================================
+ Hits         4487     4541      +54     
- Misses        288      368      +80     
- Partials       55       58       +3     

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


void ClearSearch();

bool Enabled();
Copy link
Owner

Choose a reason for hiding this comment

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

Sounds like this should be a property and not a method

public record QuickSearchFileModel
{
public string Name { get; init; }
public object Tag { get; init; }
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this property object?

public void OnCharDown(char c,
bool isShiftDown,
List<QuickSearchFileModel> files,
out bool handled)
Copy link
Owner

Choose a reason for hiding this comment

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

why out instead of returning bool from the method?

}

public void OnDataGridKeyDownCallback(Key key)
Copy link
Owner

Choose a reason for hiding this comment

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

ViewModels by definition should not have code that looks like code behind callback method. This method should be renamed to represent the business logic it contains

// "cycle from last to first"
// reset, so and start again from first
// Done in 2 'half' loops, in sake of effiency.
if (!isShiftDown)
Copy link
Owner

Choose a reason for hiding this comment

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

looks like with this logic it will iterate over the files twice?

@IngvarX
Copy link
Owner

IngvarX commented Nov 13, 2023

I will be working on refactoring this PR. It will take me at least a couple of days. Few bugs that I've noticed:

  • Sometimes key selects the first item ([..]) while it shouldn't
  • If you have no items in search Esc is the only way to close the search. If you click on other tab with mouse filtering remains the same and you have to switch back via Tab to close it. Not very convenient

TODOs left:

  • Use commands to call FPVM
  • Fix the issue with the parent directory correctly
  • Allow to Esc by clicking on the second tab
  • Add tests
  • Make sure code formatting is ok everywhere
  • Verify that dynamic creation/deletion of files when quick search is on doesn't break anything Verified: broken
  • Check if files filtering code can be improved

@IngvarX
Copy link
Owner

IngvarX commented Nov 27, 2023

Update: I managed to rewrite files filtering code but still need to address selection logic changes. I'll keep the code locally for now because it's working only partially at the moment

@IngvarX
Copy link
Owner

IngvarX commented Nov 27, 2023

Pushed a refactored revision, here is what's left:

  • Fix the issue with the parent directory
  • Allow to Esc by clicking on the second tab/when no files are found
  • Fix broken tests and add new unit/integration tests

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.

3 participants