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

New update adressing issue #7 and more #12

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ulasfo
Copy link

@ulasfo ulasfo commented Jun 9, 2024

Added two new settings to the plugin:

  1. toggle to open new .mw files under current viewed (file's) folder
  2. toggle to add different views on context menu

2nd setting semi-adresses the 7th issue. I will be adding commands to command palette as a second smaller update.

Added a new command 'markwhen-create-template':
Spawns a new file with all supported commands of
markwhenview (file is on main folder of plugin markwhen.md, new file is created and populated with its contents, could be edited easily)

Fixed a bug where spam/fast clicking open new file on ribbon would cause file creation to throw an error due to name.

Bug details: Since the name is being generated on a format with 'DD-MM-YYYY HH.MM.SS', upon clicking new file under the same second would lead the second file throw an error file already exists. This would also mean that you could generate a new file once per second. Basic solution would be to check file existance and append a number at the end of the file. The problem arises when subsequent clicks would also check for subsequent files (think of Nth file under same second, it would need to check base file, 1st extra, 2nd extra... till the N). Since file existance checks are done on disk but not on cache/ram this was a resource hog and possible disk destroyer. I rewrote the file creation system with mutexes so that after file creation it checks for the time with Date.now() and adds a number at the end of the file. Since proper usage of mutexes, the concurrency of async functions are not broken. Since properly handling the time I did not check for file existance at all and it works fine on my machine but feel free to do testing and report any bugs.

 1) toggle to open new .mw files under current viewed (file's) folder
 2) toggle to add different views on context menu

2nd setting semi-adresses the 7th issue. I will be adding commands to
command palette as a second smaller update.

Added a new command 'markwhen-create-template':
 Spawns a new file with all supported commands of
markwhenview (file is on main folder of plugin markwhen.md, new file is created
and populated with its contents, could be edited easily)

Fixed a bug where spam/fast clicking open new file on ribbon would cause file
creation to throw an error due to name.

Bug details: Since the name is being generated on a format with
'DD-MM-YYYY HH.MM.SS', upon clicking new file under the same second
would lead the second file throw an error file already exists. This
would also mean that you could generate a new file once per second. Basic
solution would be to check file existance and append a number at the end
of the file. The problem arises when subsequent clicks would
also check for subsequent files (think of Nth file under same second, it
would need to check base file, 1st extra, 2nd extra... till the N).
Since file existance checks are done on disk but not on cache/ram this
was a resource hog and possible disk destroyer. I rewrote the file
creation system with mutexes so that after file creation it checks for
the time with Date.now() and adds a number at the end of the file. Since
proper usage of mutexes, the concurrency of async functions are not
broken. Since properly handling the time I did not check for file
existance at all and it works fine on my machine but feel free to do testing and report any bugs.
Copy link
Member

@kochrt kochrt left a comment

Choose a reason for hiding this comment

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

Thank you @ulasfo for putting this together!

Ideally this PR would be split apart to so that encapsulated functionalities are addressed in individual PRs.

You should just get rid of the date in the filename, I don't like it in there anyway. Do what Obsidian does and name things "Untitled (count)".

Copy link
Member

Choose a reason for hiding this comment

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

I'm not opposed to this but I think the file should be shorter. Most people should be deleting everything anyway after creation.

Does it need to be .md or can it be .mw?

interface MarkwhenPluginSettings {
folder: string;
deftoselectedtoggle: boolean;
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 know what this variable name means - it should be camel cased

Copy link
Author

@ulasfo ulasfo Jun 17, 2024

Choose a reason for hiding this comment

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

This variable is to hold the boolean value for "open a new file in current folder (currently opened file's folder) || open a file in folder with name given in settings" hence the name "default to selected (file's folder) toggle"

interface MarkwhenPluginSettings {
folder: string;
deftoselectedtoggle: boolean;
actionToContextSettingtoggle: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Capital T actionToContextSettingToggle

const DEFAULT_SETTINGS: MarkwhenPluginSettings = {
folder: 'Markwhen',
deftoselectedtoggle: false,
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Comment on lines +39 to +44
public utilities = new class {
constructor(public superThis: MarkwhenPlugin) {
}
public testSetOuterPrivate(target: number) {
}
}(this);
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like this is being used?

Comment on lines +226 to +230
filename = filename ?? `Markwhen ${new Date()
.toLocaleString('en-US', { hour12: false })
.replace(/\//g, '-')
.replace(/:/g, '.')
.replace(/,/, '')}`;
Copy link
Member

Choose a reason for hiding this comment

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

Can we just use the Obsidian methodology and call them Untitled and not have the date in the filename?

Copy link
Author

Choose a reason for hiding this comment

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

We can in theory use untitled but still, if user spams to open file, every time we would need to check the last untitled name in the folder and add 1 to that name. I would make a toggle for this when this update is passed so that we can open files with time and/or untitled

}
}(this);

public fclass = new class {
Copy link
Member

Choose a reason for hiding this comment

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

This class should be in its own file

const countRelease = await this.countMutex.acquire();
try {
// Increment the not passed count
this.notPassedCount = 1;
Copy link
Member

Choose a reason for hiding this comment

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

The comment does not match the code, no incrementing is happening

id: 'markwhen-create-template',
name: 'Create Markwhen Template File',
callback: () => {
const markwhenfile = '/markwhen.mw';
Copy link
Member

Choose a reason for hiding this comment

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

Ensure the extensions match the actual file (.mw vs .md)

@ulasfo
Copy link
Author

ulasfo commented Jun 17, 2024

well most of the code was done in a hurry thus comment/camelcasing issues had some problems but Im glad to work on the issues. So for the next update Id be adding commands for toggling the options and switching views while also adding an option switch for obsidian-like file names untitled(count) vs date.

in future maybe we can add option to add self hosted views (url) for markwhen and use real viewer as a fallback

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