Open AlejandroSuero opened 4 months ago
I'm unsure about the implementation, mainly because you are introducing window.title
as a bool.
Now I see a parameter like this, I would love being able to use this argument to be able to set the title to anything I want like "my super app"
I'm unsure maybe window.autotitle or window.title=auto
It's just a feedback suggestion.
Also, it's a bit strange that (either):
@ccoVeille I was thinking the same, that a bool
maybe isn't the best idea a string with auto
as default I think its better too.
As for implying window
I don't really know if title
should be separated from depending on config.Window
or be a separate feature/flag where it can be shown even if controls are not shown.
For the moment, I will implement window.title=auto
and error if config.Window
is false, thus not allowing for ./freeze cut.go --window.title=auto
if no --window
is passed.
I'm fine with what you plan to do.
There is one thing that is unclear.
You said auto would be the default value.
Initially your changes weren't setting a title if none was provided.
What is current behavior and the current title? So before merging your changes.
I'm on phone, and without a computer to check
Or maybe you were saying that passing --window.title would behave as if --window. title=auto was provided
@ccoVeille I am doing the following:
if config.Window {
// same as before
titleText := config.Input
if config.Title != "auto" {
titleText = config.Title
}
} else {
if config.Title != "auto" {
printErrorFatal(...) // show that title can only be shown if `config.Window` is true
}
}
If passing --window.title
it would be the same as --window.title=auto
I think this way is better for the use with --interactive
(I am not sure if using it in interactive should be a good idea though).
Thanks, it's clearer
Anyway, I'm a bit doubtful about your pseudo code here I would expect that any config.Title would raise an error when not in window mode
I will wait for the changes to be pushed
@ccoVeille wouldn't that be what is doing in the pseudo code.
if config.Window { // window mode
} else { // not in window mode
if config.Title != auto {
printErrorFatal(...)
}
}
Or you mean another way to do it?
@ccoVeille wouldn't that be what is doing in the pseudo code.
if config.Window { // window mode } else { // not in window mode if config.Title != auto { printErrorFatal(...) } }
Or you mean another way to do it?
No my point is that if someone user --window.title=whatever
without --window
your pseudocode won't catch it.
or is it because the auto will be default value, so when not passed it would be equal to auto?
if so, then in window mode, the default value would be auto too ? so it would by default use the binary name and not the current behavior to be the input
I hope it's clearer
@ccoVeille This is the behaviour with the pseudo code implemented:
./freeze cut.go --window.title=whatever --window
./freeze cut.go --window.title=whatever
https://github.com/charmbracelet/freeze/assets/71392160/53fe7930-9333-40fd-a474-4624274616ed
One thing though is that maybe this is because I am new to using kong
but if passing --window.title
with Config.Title string
it will fail at the parsing of os.Args[1:]
but would work if --window.title=
.
Is there a way to do Config.Title string
so it will default to --window.title=auto
when passing --window.title
alone? The solution I have in my head is unmarshall it and if len(text) == 0
meaning --window.title
set the text to auto.
I'm unsure, look at the documentation, but it's simply default:"auto"
that need to be added
@ccoVeille I am gonna search through the documentation, but I have default:"auto"
added and still didn't work.
In the meantime I am doing this:
args := os.Args[1:]
for i, arg := range args {
switch arg {
case "--window.title":
if i+1 < len(args) {
if strings.HasPrefix(args[i+1], "--") || strings.HasPrefix(args[i+1], "-") { // check if next arg is a flag
args[i] = "--window.title=auto"
}
} else { // if no more args `--window.title` set to default "auto"
args[i] = "--window.title=auto"
}
break
case "--window.title=": // if window title equals to ""
printErrorFatal("Invalid argument", errors.New("Use --window.title or --window.title=auto instead"))
}
}
ctx, err := k.Parse(args)
This is ugly and should be avoided
I cannot urge yourself to do it in another way
This is ugly and should be avoided
I cannot urge yourself to do it in another way
I know, as of right now what I got is:
type Config struct {
// ...
Title string `json:"title,omitempty" help:"Display input file as title. {{--window.title=auto}} as default" prefix:"window." default:"auto" group:"Window"`
// ...
}
But this results in the error:
> ./freeze cut.go --window --window.title
ERROR Invalid Usage
--window.title: expected string value but got "EOL" (<EOL>)
or
> ./freeze cut.go --window.title --window
ERROR Invalid Usage
--window.title: expected string value but got "--window" (long flag); perhaps try --window.title="--window"?
@ccoVeille if using it like ./freeze cut.go --window
the title will show with the default of auto
if that is the desired behaviour, it works like that for now. To remove the title it will be just --window.title=
and it will display ""
as of nothing.
Here is the current state of the behaviour:
https://github.com/charmbracelet/freeze/assets/71392160/5c9568e2-ee4b-42a4-bdf4-0c0d126114ae
[!NOTE] The amount of changes is because
make golden
now that--window
also adds the title filename as title by default.
I saw you opened https://github.com/alecthomas/kong/discussions/432
Let's wait
@ccoVeille yes, let's see if it can be use as a bool
type like --window.title
or it's just the behaviour expected from a string --window.title=auto
when not included.
I stashed the changes for the time being 😁
@ccoVeille from the discussion on https://github.com/alecthomas/kong/discussions/432 it seems that is not possible to do freeze cut.go --window --window.title
unless it's a bool.
One option to solve this if showing the input file as the title is not an option, could be to do the following:
type Config struct {
// ...
Title bool `json:"title ... prefix:"window."`
TitleName string `json:"titleName .... prefix:"window." default="auto"`
// ...
}
If config.Window
and config.Title
, show config.TitleName
which if "auto"
will show the filename and if explicitly changed, will use the user's input for the title.
if config.Window {
// ...
if config.Title {
title := config.Input
if config.TitleName != "auto" {
title = config.TitleName
}
// add title to image
}
} else {
if config.Title || config.TitleName != "auto" {
// error for invalid usage since depends on `config.Window = true`
}
}
This seems too complicated. This feature would require 2-3 parameters.
Also, it's uneasy for me to review pseudo code that I cannot fetch/test/help you to improve.
I think it's important to list the things you want to achieve:
when window is provided
when window is not provided:
Am I missing something?
Based on this and the fact you are struggling to implement it in an easy way with Kong.
If we add the fact that user needs to provide an additional parameter to set his title to something that will be the filename they already provided, and the fact we can assume most people may also change the title directly by reading the documentation (if they do)
I would recommend to simplify the implementation
when window is provided
when window is not provided
I think it would allow to cover anyone need
@ccoVeille yes, this is what I meant with my pseudo code, it is too convoluted and over-complex to implement a way to set title or not that way.
I will commit the changes I made using config.Title string
that follows the next implementation.
Implementation:
--window
provided:
--window.title
not provided, set input filename as title.--window.title
provided and not empty, set title.--window.title
provided and empty, do not set title.--window.title
provided, report error.With this implementation it will lead to the following usage:
--window
provided
freeze cut.go --window
-> title: cut.go
.freeze cut.go --window --window.title "my title"
-> title: my title
.freeze cut.go --window --window.title=
-> don't set title.--window
not provided
freeze cut.go --window.title "my title"
-> report bad usage errorAnd the user can set in --interactive
for example window.title = ""
so it won't show by default when using --window
if they don't want to use this behaviour by default.
This will leave the following:
Title string json: ... default="auto"
so it will default to filename as title when only providing --window
. @ccoVeille I added the changes with test, if you want to try them out and give me your opinion.
@ccoVeille I made the changes corresponding with your review. Nice feedback as always 😉
[!WARNING] This could be added after in another PR if users want it and leave this PR as it is.
@ccoVeille one idea that popped when I was walking my dog (random fact) is the ability to decide the position of the title.
// config.go
type Config struct {
// ...
Title Title `json:"title,omitempty" help:"..." embed="" prefix:"title."`
// ...
}
type Title {
Name string // the now implemented `--window.title`
Position string `json:"position" help:"... options {{left|center|right}}" default:"center"`
}
// svg.go
func NewWindowTitle(..., pos string) (*etree.Element, error) {
// if not "center" or "left" or "right", report error
// if "center" how is now behaving
// if "rigth" set the text aligned to the right
// if "left" set the text aligned to the left after the window controls
}
[!NOTE]
This will change from
--window.title
->--title.name
Example:
freeze cut.go --window
______________________________________
| O O O title |
| |
| # code |
|______________________________________|
freeze cut.go --window --title.position=left
______________________________________
| O O O title |
| |
| # code |
|______________________________________|
freeze cut.go --window --title.position=right
______________________________________
| O O O title |
| |
| # code |
|______________________________________|
It would make sense 🐶
@ccoVeille I implemented it with --title.position
as an int
where 0 = left
1 = center
2 = right
, it could be changed to "left"|"center"|"right"
, whatever seems best.
./freeze cut.go --window --title.position=1 --title.text "my long custom title" --output middle.png
:./freeze cut.go --window --title.position=0 --title.text "my long custom title" --output left.png
:./freeze cut.go --window --title.position=2 --title.text "my long custom title" --output right.png
:[!NOTE]
With this approach I also realised that we can include
config.Title.Show
to use--title.show=false|true
to hide or show the title easily.This can lead to being able to use title without the need of
--window
if we want to add that behaviour.
it could be changed to \"left\"|\"center\"|\"right\", whatever seems best.
It should be changed yes, magical number makes no sense here
@ccoVeille I added the changes to work with --title.position="{left|center|right}"
.
I tried to simplify a bit more the arguments of NewWindowTitle
.
@ccoVeille I made the changes, tell me if I left something out from the feedback you gave me.
@ccoVeille I made the changes according to your last feedback.
For some reason I thought you meant unexported fields in my head and the brain wasn't braining correctly in that moment 😅
@ccoVeille can you test something?
When I am in my branch and I compile and run ./freeze
it will generate the image as expected, but in the README.md
the title is slightly off.
Locally:
In the README.md from branch:
I don't know if it just working like it should and this is some weird behaviour from github or what, but if working locally for everybody else, we shouldn't change it right?
[!NOTE]
Slightly off-topic but I thought that when using
--execute
should be nice to strip.ansi
from the title, what do you think.
I cannot test now and for the next 3 days, as I'm not at home and with a phone only.
Could it be a font thing? Are the font available on the GHA docker images?
@ccoVeille I am using the default font (JetBrains Mono) where can I check for the GHA docker images, to see if it is using the same font?
I made some testing with and without librsvg
installed:
[!NOTE] Image on the top is
.svg
, on the bottom is.png
Command used:
./freeze test/input/artichoke.hs --border.width 1 --border.color "#515151" --border.radius 8 --window --font.family "JetBrains Mono"
(--output freeze.svg
for.svg
)
With:
Without:
Seems like librsvg
and the package resvg-go are doing transformations differently, I can try a workaround tomorrow to try adjusting the height depending on if rsvg-convert
is or isn't installed.
Oh I'm seeing your few months old message only now. Did you try to see why it differs?
@ccoVeille tbh, I couldn't find why it is, at least to my knowledge. Because if I am using the same font locally, the resulting image should have the same font both with and without libsrvg
installed, but seems like it changes the font.
Ok, it happens. Font world is the CSS part of image: Bermuda Triangle
@ccoVeille yeah, being a wannabe web-dev myself I know what you mean.
I will be testing this branch tomorrow in case some things need some tweaks or minor fixes, to have it in a state that works, maybe reducing the font size of the title at least makes it not stand out as much.
With these changes when
--window=true
if--window.title=true
it will display the input filename as the title of the screenshot../freeze cut.go --window --window.title
make test
on MacOS 14.5 (23F79)Closes #89.