-
Notifications
You must be signed in to change notification settings - Fork 32
Inline Mode Core Patches #10
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
base: master
Are you sure you want to change the base?
Conversation
Changed viewer height to 100%
Inline Mode Core Patches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for the contribution! I appreciate the time you put into trying to fix this problem!
The main issue with inline timestamps is I'd like all timestamps to be able to be viewed immediately in "live preview mode." If you could find a way to make inline timestamps work without having to switch between 'live-preview' and 'reading' modes, that would be the optimal solution.
If not, the second best option is having non-inline timestamps work like normal (appearing immediately in live-preview mode) and inline timestamps appearing as code-blocks up until a user switched into 'reading mode.' The main problem with this solution is that the user may think the plugin is broken if they see a code-block instead of a button when making an inline-timestamp for the first time. Perhaps add a message (that only appears in the code block) explaining that the inline button will appear once they switch to 'reading' mode.
Please combine the inline functionality into the main timestamp command as I'd like to keep it to one command per function. However, please make sure this does not break previous timestamps created by users.
Thanks again for the work you put into improving this plugin. Much appreciated!
| const time = (hours > 0 ? leadingZero(hours) + ":" : "") + leadingZero(minutes) + ":" + leadingZero(seconds); | ||
|
|
||
| // insert timestamp into editor | ||
| editor.replaceSelection("```timestamp \n " + time + "\n ```\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is breaking the timestamp button in "live preview mode."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if what you are asking is possible for inline stuff.
The icon plugin operates the same way in live preview mode.
blacksmithgu/obsidian-dataview#1058 this also suggests its treated specially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so a quick check and this is not the reason it doesn't work in live preview.
The issue is that the MarkdownPostProcessor handler is ONLY called in reading mode.
Inline code blocks ( with the cm6 editor ) are handled differently. The only way to make this work for live preview is to write a CodeMirror 6 decoration.
https://github.com/nothingislost/obsidian-cm6-attributes
Given I'm not even sure I will take notes on videos and that I don't use live preview, implementing this won't make it to the top of the todo pile. Sorry.
main.ts
Outdated
| if (!this.player) { | ||
| editor.replaceSelection(ERRORS["NO_ACTIVE_VIDEO"]) | ||
| return | ||
| } | ||
|
|
||
| // convert current video time into timestamp | ||
| const leadingZero = (num: number) => num < 10 ? "0" + num.toFixed(0) : num.toFixed(0); | ||
| const totalSeconds = Number(this.player.getCurrentTime().toFixed(2)); | ||
| const hours = Math.floor(totalSeconds / 3600); | ||
| const minutes = Math.floor((totalSeconds - (hours * 3600)) / 60); | ||
| const seconds = totalSeconds - (hours * 3600) - (minutes * 60); | ||
| const time = (hours > 0 ? leadingZero(hours) + ":" : "") + leadingZero(minutes) + ":" + leadingZero(seconds); | ||
|
|
||
| // insert timestamp into editor | ||
| editor.replaceSelection("`:vts=" + time + "`\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please merge the vts code into the timestamp code to reduce code repetition. I'd prefer to keep this DRY and I would like one button for inserting a timestamp, instead of two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was what I was talking about with the refactoring required when I shared the code and comments on the main issue.
Are you suggesting replacing the original function and entirely operating inline ? I didn't swap it out as I presumed someone would want it to operate the old way also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the functionality of inserting timestamps and inline timestamps is the same (both insert a timestamp button), I'd like to have one command for either variant.
Basically, the function should insert an inline timestamp if there is text before or after the insertion place and insert a normal timestamp otherwise. Preferably, both inline and normal timestamps should work in 'live preview mode.' This was my main issue, which is why I didn't include inline-functionality when I released this. But, since inline buttons are a very useful feature to have, I'd accept a solution where:
- normal timestamps appear immediately in live-preview mode (normal functionality)
- inline-timestamps appear as code block in live preview mode (preferably with a disclaimer in the code block telling the user that the button only appears in 'reading mode') and as buttons in 'reading mode'
- both inline and normal timestamps are triggered by the same command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, your suggestion makes sense, but would break my potential workflow.
I originally implemented this for tidy timestamps in source mode. I may or may not use them as inline, but I will always use them in preference to the original code block mode.
I would want to be able to place a single timestamp on its own line and for that to work. This flexibility and choice was why I added a second function rather than replacing the original in my code.
The DRY part is what I was calling out as needing some extra work.
main.ts
Outdated
| import { VideoView, VIDEO_VIEW } from './view/VideoView'; | ||
| import { TimestampPluginSettings, TimestampPluginSettingTab, DEFAULT_SETTINGS } from 'settings'; | ||
|
|
||
| import { MarkdownRenderChild } from "obsidian"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is currently being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly not - I grabbed it from a snippet when I was working out the alternative renderer.
The code may benefit from a little refactoring, but adds support for creating and rendering inline timestamps.
`:vts=HH:MM` - Button title`Links to comments on issue #9.
Examples of rendering: