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

Proposed solution to #746 : Allow disabling the git-commit-routine via hotkey-toggle #834

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

Conversation

Gewerd-Strauss
Copy link

Hello,

this is a proposed solution to the issue outlined in #746, allowing the user to temporarily suppress the plugin committing files via a hotkey-toggleable config-setting.
I remain that I have not found any issues arising from these changes on my end. I've tried to implement the solution comparable to existing code.

I am open for suggestions/review.

Thank you.
Sincerely,
~Gw

@Gewerd-Strauss Gewerd-Strauss force-pushed the feat/suggested-solution-to-#746 branch from 662cf27 to bd3b968 Compare January 6, 2025 10:00
@Gewerd-Strauss
Copy link
Author

I have since resolved the merge conflicts, but I am not entirely sure if they should be resolved this way. I'd leave this up to maintainers, and can revert these changes for manual review.

Copy link
Author

@Gewerd-Strauss Gewerd-Strauss left a comment

Choose a reason for hiding this comment

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

Note that I am not sure if this callback must be asynchronous or not, this is not my specialty.

@Vinzent03
Copy link
Owner

Vinzent03 commented Jan 11, 2025

I've changed quite a bit of stuff now. It's no longer persisted to settings, but the current session only. Does this still fit your needs and works how you would expect?

@Gewerd-Strauss
Copy link
Author

Hmm.

Does this still fit your needs and works how you would expect?

I am not sure.

On the one hand this is a good idea, because if the user relies on the plugin for backup-purposes they won't all-too-bad sudden realisations of 'oh, I haven't committed in ten days'.

What I am not sure about is if this new method would also suppress the execution of calls to git when the boolean is changed while an async to asny commit() is scheduled (aka: does an async routine respect object updates issued after the routine's async call was set up? I don't know typscript's intrinsics to that point.)

A good thing about the initial design was that if I disabled the plugin by a config-setting which was queried during commit():

if (!(this.settings.temporaryDisableAutomatics)) return false; // if temporaryDisableAutomatics-checkbox is disabled, return without committing

it would persist over resets. There are basically two use-cases for this functionality (at least on my end):

  • temporarily disable automatic commit/push/pull while working in f.e. VS Code/wanting to curate commits myself/..., and re-enable it afterwards. Since obsidian's wealth of plugins can sometimes hinder work in other programs if they simultaneously operate on the same 'vault' (think stuff like auto-note-mover, linter, probably a couple other as well), there can still exist reasons beyond obsidian-git that might cause the user to temporarily close Obsidian. I feel like then re-opening obsidian and git coming back to life might be a source of frustration.
  • have automatics disabled across obsidian restarts allows me to use obsidian even after closing it, without having to worry about suddenly re-disabling it. Essentially, this made it an explicit master toggle, so you knew that if you disabled it, you also had to enable it again.

From a design perspective I feel like this explicit manner doesn't mix well with a 50%-automation? (Please excuse me for this probably clunky explanation, I don't really know how to voice this effectively for some reason. I hope you get the gist.)

In principal I like the solution you propose, I just don't like the mix-up it implies.


A potential middle-ground between both could be the implementation of some grace-period and a warning to the user, enacted/issued upon restart?
Such that when opening Obsidian while the last session was closed with automatics inhibited, the user would get some notice Last session was closed with 'obsidian-git' disabled. Automatic routines will resume in X seconds, unless automatics are disabled <by hotkey/methodX/methodY/methodZ> beforehand..

That would resolve the danger of users forgetting their git routines are not working (and thus, they are not backing up/syncing/...), and allow pseudo-persistent deactivation across app-restarts.

And if the use re-enables automatics before closing, said notices would obviously not trigger.


Does this make sense?

@Gewerd-Strauss
Copy link
Author

As I said, I am unsure if the changes you propose make the solution better, or not. In parts because I am not that well-versed in typescript's inner workings and thus have a harder time judging it.

Could you provide feedback on these thoughts/the middle-ground I propose?

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.

2 participants