feat(#89): added input file as window title

[AlejandroSuero]

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

• tested with make test on MacOS 14.5 (23F79)

Closes #89.

[AlejandroSuero]

Posible changes

• Change the font weight to a bold one
• Make font size slightly larger

[ccoVeille]

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):

• window.title doesn't imply window
• window.title can be passed as an argument, and nothing is reported if window is not provided

[AlejandroSuero]

@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.

[ccoVeille]

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

[ccoVeille]

Or maybe you were saying that passing --window.title would behave as if --window. title=auto was provided

[AlejandroSuero]

@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).

[ccoVeille]

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

[AlejandroSuero]

@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]

@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

[AlejandroSuero]

@ccoVeille This is the behaviour with the pseudo code implemented:

• ./freeze cut.go --window.title=whatever --window
  

• ./freeze cut.go --window.title=whatever

freeze-windowtitle-nowindow.mp4

[AlejandroSuero]

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.

[ccoVeille]

I'm unsure, look at the documentation, but it's simply default:"auto" that need to be added

[AlejandroSuero]

@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)

[ccoVeille]

This is ugly and should be avoided

I cannot urge yourself to do it in another way

[AlejandroSuero]

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"?

[AlejandroSuero]

@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.

[AlejandroSuero]

Here is the current state of the behaviour:

freeze-window-title-demo.mp4

Note

The amount of changes is because make golden now that --window also adds the title filename as title by default.

[ccoVeille]

I saw you opened alecthomas/kong#432

Let's wait

[AlejandroSuero]

@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 😁

[AlejandroSuero]

@ccoVeille from the discussion on alecthomas/kong#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`
  }
}

[ccoVeille]

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

  • have the text input when no optional parameter is provided
  • being able to ask to use the binary name
  • being able to set an empty title

• when window is not provided:

  • but any optional parameter is provided, report an error

Am I missing something?

[ccoVeille]

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.title is provided and not empty, set the title
  • when window.title is provided and empty, remove the title

• when window is not provided

  • when window.title is provided, report an error

I think it would allow to cover anyone need

[AlejandroSuero]

@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 not provided
  • --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 error

And 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.

[AlejandroSuero]

@ccoVeille I added the changes to work with --title.position="{left|center|right}".

I tried to simplify a bit more the arguments of NewWindowTitle.

[AlejandroSuero]

@ccoVeille I made the changes, tell me if I left something out from the feedback you gave me.

[AlejandroSuero]

@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 😅

[AlejandroSuero]

@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.

[ccoVeille]

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?

[AlejandroSuero]

@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.

[ccoVeille]

Oh I'm seeing your few months old message only now. Did you try to see why it differs?

[AlejandroSuero]

@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.

[ccoVeille]

Ok, it happens. Font world is the CSS part of image: Bermuda Triangle

[AlejandroSuero]

@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.