tooltips and persistent settings for export options, added VTT support #124
Conversation
|
Have not verified VTT output works properly, need to find something for validating. SRT coordinate widgets are still rendering funny. |
|
also sorry, looks like my autoformatting made a bunch of non-edit edits |
|
at a quick glance it looks like one big auto-formatting change is from two space indentation to four? maybe you can tweak that setting back to two to remove some of the unrelated diff noise? |
|
Playing around with a validator, I'm seeing a fundamental issue with our idea of subtitle export: Might a better approach be to make subtitle files that are just the name of the source clip + subtitle extension? JSON and CSV may be an entirely different use case, but I think this is a big quirk right now... Another very small UI suggestion I have for this related to our discussion of getting rid of modals is to remove the "success" dialog and just disappear the export dialog if successful. |
There was a problem hiding this comment.
sorry this review is taking so long - trying to put an honest effort into understanding all the changes correctly.
also a bit curious about the commit history on this pr? it seems a bit odd that it shows all of those historical merge commits as part of this branch specifically? 🤔🤔🤔🤔🤔
might be worth it to apply these changes to a clean branch, especially after turning off some of your auto-formatting settings. 🤷🏻♂️
| if is_selected then | ||
| ImGui.SetItemDefaultFocus(ctx) | ||
| function TranscriptExporterFormats:init() | ||
| if not self.format_widget then |
There was a problem hiding this comment.
init will only be called once so i don't think this block needs to be wrapped
| if not options.json_widgets then | ||
| local storage = Storage.ExtState.make { | ||
| section = 'ReaSpeech.Export.JSON', | ||
| persist = true | ||
| } | ||
|
|
||
| options.json_widgets = { | ||
| object_per_segment = ReaSpeechCheckbox.new { | ||
| state = storage:boolean('object_per_segment', false), | ||
| label_long = 'One Object per Transcript Segment', -- Changed from label to label_long | ||
| help_text = [[Each transcript segment is exported a separate JSON object.]] | ||
| } | ||
| } | ||
| end | ||
|
|
||
| Trap(function() | ||
| options.json_widgets.object_per_segment:render() | ||
| options.object_per_segment = options.json_widgets.object_per_segment:value() | ||
| end) |
There was a problem hiding this comment.
i think maybe it might be best to just call render instead of also setting the value by calling the widget value with each render cycle. maybe we create another group of functions that represent each of these "if not options.*_widgets" blocks:
function TranscriptExportFormat.widgets_json(options)
if options.json_options then
return options.json_options -- already initialized
end
local storage = Storage.ExtState.make {...}
options.json_options = {
object_per_segment = ReaSpeechCheckbox.new {...},
}
return options.json_options
end
function TranscriptExportFormat.options_json(options)
Trap(function()
local widgets = TranscriptExportFormat.widgets_json(options)
widgets.object_per_segment:render()
end)
end
function TranscriptExportFormat.writer_json(transcript, output_file, options)
Trap(function()
local widgets = TranscriptExportFormat.widgets_json(options)
if widgets.object_per_segment:value() then
...
else
...
end
end)
end
also kinda nice benefit to keep the persistence/widget definitions separate from the rendering stuff. this all gets super noisy so it's a challenge to keep it understandable (clearly, right? lol).
i've definitely wondered about this before. our main one-big-blob-of-transcript data model kinda leaves a bunch of questions like this open.
that could at least be a nice option maybe? like, a checkbox "per source file" that un-disables a set of file naming options?
on another branch (#115) i'm working on providing a "reveal" command. maybe on success it says "Export successful" with a "reveal" link but the notice can...disappear automatically after a short interval. 🤔 |
No description provided.