Status bar icon toggle #91
Conversation
… in setting issue
… file being viewed
|
@lostintangent just a follow up comment to see if you have any interest in merging these changes? Cheers |
|
i would also love seeing this merged, since i have the same problem described in #90 |
|
@lostintangent if no response soon I think I'll pop the fork up on the vs code extension store. |
|
@tomglynch hey apologies! I'll review this tomorrow and we can get it merged this week |
lostintangent
left a comment
lostintangent
left a comment
There was a problem hiding this comment.
Thanks for this! My comments below are essentially just thoughts on how to simplify the implementation, since I think we may be duplicating logic that we don't need. But I'm out of town and haven't been able to actually run/debug the code. So I'm just thinking out loud 👍
Additionally, since this extension has millions of installs, and this PR is making a notable UX change, we should add a setting to control whether to always show the status bar or not. And default it to false. That way the current behavior doesn't change, but you and others can opt into having the status bar always visible.
src/extension.ts
Outdated
| import * as minimatch from "minimatch"; | ||
|
|
||
| // Helper function for file pattern matching | ||
| function matches(uri: vscode.Uri) { |
There was a problem hiding this comment.
Instead of duplicating this function, we should define it as a reusable utility that can be imported in this file and in the watcher.
There was a problem hiding this comment.
Or just export the function from the watcher module and then reuse it here
src/extension.ts
Outdated
| } | ||
|
|
||
| // Initialize the store and context based on the configuration | ||
| const initialEnabled = vscode.workspace.getConfiguration('gitdoc').get('enabled', false); |
There was a problem hiding this comment.
The previous code was handling this logic already, since "config.enabled" pulls its value from the workspace configuration. Is there a reason we need to change it to this more verbose implementation?
src/extension.ts
Outdated
| // Initialize the store and context based on the configuration | ||
| const initialEnabled = vscode.workspace.getConfiguration('gitdoc').get('enabled', false); | ||
| store.enabled = initialEnabled; | ||
| updateContext(initialEnabled, false); // Set initial context to match config |
There was a problem hiding this comment.
updateContext will already be run in response to setting store.enabled, so we don't need this. And then related to my previous comment, I think we can just revert this entire hunk.
src/extension.ts
Outdated
| () => checkEnabled(git) | ||
| () => store.enabled, | ||
| (enabled) => { | ||
| updateContext(enabled, true); |
There was a problem hiding this comment.
We shouldn't need this since checkEnabled will already update the context as needed.
src/extension.ts
Outdated
| (enabled) => { | ||
| updateContext(enabled, true); | ||
| checkEnabled(git); | ||
| debouncedUpdateStatusBar(vscode.window.activeTextEditor); |
There was a problem hiding this comment.
I think this should be handled by the checkEnabled function, and effectively revert to the previous code. That way this reaction is simply watching for enabled changes and then updating the UX based on that (via checkEnabled)
There was a problem hiding this comment.
Actually, you already have a reaction in the watcher module that will update the status bar when the store.enabled changes. So I think we can just remove this?
|
Thanks for all your comments @lostintangent! I agree with everything and have applied them to my latest commit.
The changes maintain backward compatibility while allowing users to opt into the new behavior. Let me know if you'd like me to make any additional adjustments. I also added a few tests that I haven't pushed yet if you'd like to take a look - but perhaps that's a seperate PR. |
|
@lostintangent Thanks for your work on this! Any chance to get this PR merged? I think a couple of people are affected by #90. |
|
I'm having the same issue |
|
Would love to see this merged. Currently resorting to a fork to use this in courses to avoid #90. |
|
I am also affected by this. +1 for merging :) |
9 participants
Hi @lostintangent, hope you're doing well. GitDoc is a great tool, but I wanted a feature to have it always show the status bar icon rather than it disappearing when disabled (#89 ). In fixing that, I also ended up fixing #90 as it affected the icons being shown. I handled icons for matching the File Patterns too. Have a look and let me know what you think?
Cheers,
Tom