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

some changes to make it work like Emacs vertical file completion. #185

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

Conversation

yangwen0228
Copy link

image

image

@skuroda
Copy link
Contributor

skuroda commented May 6, 2022

Hey @yangwen0228 , just as a note I do see these coming in but have been busy. I appreciate you enhancing this. I see you're iterating on this. Can you add a comment when you're ready for this to be merged (and casually reviewed). I'm not up to date on the latest sublime text APIs so if you're using any of those, I'll rely on trusting what you're doing :D

@yangwen0228
Copy link
Author

OK! I find your package is the best one in the Package Control to handle the file operation. And I enhance this package to almost the same experience in Emacs. Emacs is very handy and extensible, but the speed is not quite good when use in company computers with disk encrypt software. I use this package after enhanced for hours. It is becoming handy now. But it still needs some improvements to increase the large project files sorting performance, and increase the stability when popup frequently. Maybe I should expend some time to research the partial APIs. I use the Sublime Text4, but not test in the old versions. Just wait the weekend to improve this.

@yangwen0228
Copy link
Author

Hey @yangwen0228 , just as a note I do see these coming in but have been busy. I appreciate you enhancing this. I see you're iterating on this. Can you add a comment when you're ready for this to be merged (and casually reviewed). I'm not up to date on the latest sublime text APIs so if you're using any of those, I'll rely on trusting what you're doing :D

I think everything works as expected, now! Do you want to have a try? Or just merge in?

@skuroda
Copy link
Contributor

skuroda commented May 22, 2022

Hey @yangwen0228 catching up on this. In some chats with the SublimeText organization, we're going to move this project into their space. I'll likely let them help manage the approvals as well (because they are far more up to date than I am with practices around plugins today). I do notice the removal of some of the settings. What's the impact to those that that either used the default false value, or had it as true?

@yangwen0228
Copy link
Author

Great! No problem!

Copy link
Member

@FichteFoll FichteFoll left a comment

Choose a reason for hiding this comment

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

I took a not so brief look as I initially intended at this and have added a few comments with some questions as well as suggestions.

Generally, I think adding a completion list is a good idea, but this is going into the wrong direction because it introduces a UX foreign to ST in general. Using the native completions feature would feel much more natural and should work better even, though it may require some more detailed knowledge on how the corresponding API needs to be handled. I wish I had the time to work on it, however, because it sounds nice.

The PR also breaks compatibility with both ST2 and ST3 at the moment and is missing a .python-vesion file to declare the plugin host since it uses 3.6+ syntax. I would be very much fine with dropping compatibility for new code & features, but moving unsupported versions to mainenance mode needs to be mentioned in the readme. (This is a heads-up for @skuroda.)

Comment on lines +537 to +540
if "/" in path:
return path
else:
return ""
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this logic here, but the return value doesn't seem to be used anyway?

Comment on lines +19 to +21
print(self.settings)
completion_delay = self.settings.get(COMPLETION_DELAY_SETTING, 100)
print(completion_delay)
Copy link
Member

Choose a reason for hiding this comment

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

Debug prints

else:
choices_ratio = {}
for choice in choices:
choices_ratio[choice] = levenshtein_ratio(query, choice)
Copy link
Member

Choose a reason for hiding this comment

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

Better use a dict comprehension now.

@@ -17,9 +17,6 @@
// A default initial value to fill the create new file input path with.
"default_initial": "",

// When renaming a file it will we pre populated with the file existing filename.
"autofill_path_the_existing": false,
Copy link
Member

Choose a reason for hiding this comment

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

At first I questioned why the setting was removed, but in hindsight it never made it into a release (a5f062b), the name is questionable, I'm not sure if the code even works, and the behavior is somewhat as well, though I can see the appeal. Regardless, the opposite should have been the default.

A mention of that in the commit message (and having it in a separate comment) would have been nice, though.

{"keys": ["ctrl+p"],"command": "advanced_new_file_prev",
"context": [{"key": "setting.anf_panel"}]
},
{"keys": ["ctrl+l"],"command": "advanced_new_file_updir",
Copy link
Member

@FichteFoll FichteFoll Jun 11, 2022

Choose a reason for hiding this comment

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

Why L? Is that how emacs does it? Please add some comments above the binding to explain their inspiration because I (for example) wouldn't know otherwise. Probably to the readme as well.

Comment on lines +21 to +23
{"keys": ["enter"],"command": "advanced_new_file_confirm",
"context": [{"key": "setting.anf_panel"}]
},
Copy link
Member

Choose a reason for hiding this comment

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

Will this prevent submitting the inserted new file name when the completion list is not visible? It would also not match the native completions UX if "auto_complete_commit_on_tab": true.

Comment on lines +288 to +301
def clear_input_view(self):
self.get_active_view_settings().erase("anf_input_view")

def set_input_view_content(self, content):
self.get_active_view_settings().set("anf_input_view_content", content)

def get_input_view_content(self):
return self.get_active_view_settings().get("anf_input_view_content")

def clear_input_view_content(self):
self.get_active_view_settings().erase("anf_input_view_content")

def clear_input_view_project_files(self):
self.get_active_view_settings().erase("anf_input_view_project_files")
Copy link
Member

Choose a reason for hiding this comment

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

Imo these functions provide barely any value over their bodies. Besides, I'm unsure what the purpose of get_active_view_settings is when in reality it falls back back to the window's settings?

If you want to store data per window, you should probably do that explicitly (and all the time instead of only sometimes). However, I'm unsure if a window's settings are persisted on disk on exit, so this may actually result in leaking outdated data. An alternative would be a separate mapping that associates window-specific data with the window's id and is sufficiently cleared when the panel is closed. Usually I'd suggest storing that in the window command's instance variables, but since this plugin uses multiple commands that's not really an option.

try:
input_view.hide_popup()
except Exception as e:
print("hide_popup", e)
Copy link
Member

Choose a reason for hiding this comment

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

Debug print?

advanced_new_file/completions/fuzzy_sort.py Show resolved Hide resolved
from ..anf_util import *


class AdvancedNewFileProjectFileCommand(AdvancedNewFileNew, sublime_plugin.WindowCommand):
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this command?

@skuroda
Copy link
Contributor

skuroda commented Jun 15, 2022

I took not so brief look as I initially intended at this and have added a few comments with some questions as well as suggestions.

Generally, I think adding a completion list is a good idea, but this is going into the wrong direction because it introduces a UX foreign to ST in general. Using the native completions feature would feel much more natural and should work better even, though it may require some more detailed knowledge on how the corresponding API needs to be handled. I wish I had the time to work on it, however, because it sounds nice.

The PR also breaks compatibility with both ST2 and ST3 at the moment and is missing a .python-vesion file to declare the plugin host since it uses 3.6+ syntax. I would be very much fine with dropping compatibility for new code & features, but moving unsupported versions to mainenance mode needs to be mentioned in the readme. (This is a heads-up for @skuroda.)

Also good with putting ST2 and 3 versions of the plug in into maintenance mode. I vaguely recall the package control entry having something to pin the versions there (for ST2 at the time). Am I correct in assuming that's the same action that needs to happen still?

@FichteFoll
Copy link
Member

Am I correct in assuming that's the same action that needs to happen still?

Yes, it still works the same way. The suggested course of action is to create a prefixed tag for the legacy version (e.g. st3-1.7.0 which would just be a re-tag of the current latest version) and then split the release channels in two, one for ST3 and older with "tags": "st3-" and the other already existing one for the latest versions (can remain unchanged as "tags": true.

@skuroda
Copy link
Contributor

skuroda commented Jul 16, 2022

Tag pushed and wbond/package_control_channel#8571 to pin the version.

@skuroda
Copy link
Contributor

skuroda commented Jul 16, 2022

I've merged in a couple readme updates in #186 that I'm guessing will conflict with some of the changes in this PR.

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