Closed peterhellberg closed 6 years ago
I would be happy to send a PR for this if you think it is a good idea to begin with @faiface
I'm a fan of the Reset
method and BasicAtlas
. Printf
, not so much. I consider it an advantage that the interface, io.Writer
, is separated from the higher-level uses of it, such as fmt.Printf
. Adding a Printf
method would unnecessarily duplicate code and API and make the thing more complex in the long run.
And yeah, not sure if BasicAtlas
is the right name. I feel like 7x13
should be somewhere in the name. BasicAtlas7x13
? Atlas7x13
? Something like that
Taking my comments into account, I'll be happy to accept a PR :)
The *text.Text.Printf
would just call fmt.Printf
, so not much duplication.
(And it would match *log.Logger.Printf)
Not much code duplication, but an API duplication. Two ways to do one thing. One thing I really love about Go is that it's avoiding this "there are many ways to solve each problem" thing. I think that's great. It removes mental friction and let's you focus on the problem at hand instead of figuring out which one of those many solutions is the best at any given moment. Furthermore, *log.Logger
is not an io.Writer
for some reason.
Here is some code that I'd like to improve after doing these changes: https://gist.github.com/peterhellberg/6b7915e9c51c44b8d7e21112cbdf8d0f#file-pixel-pong-go-L146-L148
Eg. I'd love a way to do p.txt.<FunctionName>("% 3d", p.score)
since constantly replacing the contents of a *text.Text
should be a pretty common use case?
I get that. But, the difference between fmt.Printf(p.txt, ...)
and p.txt.Printf(...)
is just 6 characters, not much. I simply consider adding (*Text).Printf
and related functions a little ugly and not worth addition. The reason is based on a principle: separation of concerns is good and should be embraced. Here, the concern of *text.Text
is to implement the interface, io.Writer
, while the concern of the fmt
package is to utilize this interace to its maximum potential.
Imagine we added (*Text).Printf
, (*Text).Print
and (*Text).Println
functions. Now, unrealistic, but let's imagine that the fmt
package added a new printing function, say fmt.Printz
with some special printing mechanism. How would our code look like after this change to the fmt
package?
txt.Printf("%d %d", 1, 2)
txt.Println("Hello, world!")
txt.Print("Yay")
fmt.Fprintz(txt, ...)
Inconsistent, you see? In order to make it consistent again, we'd have to add the Printz
method to the *Text
type. Changing same thing in two places, that's duplication, that's not DRY.
Of course, it's highly improbable that the fmt
package would add a method, but I think that these principles should be obeyed even in situations like this.
Ok. I'll listen to what you want, even though I don't agree with the premise that adding support for writing a formatted string to this package would lead to unchecked duplication/feature creep.
The Printf(format string, a ...interface{})
signature is hardly novel, especially not in packages dealing with writing strings.
(We already have convenience methods like WriteString
, WriteByte
and WriteRune
, and fmt
is already imported in the text package)
Should I remove the unused f2i
function while I'm at it?
Yeah, no problem. Btw, it just hit me that Reset
is probably not the right name, because IMDraw has a Reset function which does something different (resets all properties, such as Color, etc.) and that might be confusing.
How about Erase
or Empty
instead?
Erase
or Empty
is not really distinguished from Clear
. I was thinking, maybe ClearAndReturn
? Not sure if 'return' is the right word, but could be.
Is there a use case for Clear
to NOT set txt.Dot = txt.Orig
btw?
Yeah, when you want to show some text parts by parts. Say word by word, one word at a time. Although that's much less used than Clear
followed by txt.Dot = txt.Orig
. It might make sense to make txt.Clear
reset the dot automatically and then if you wanted to preserve it, you'd do this:
dot := txt.Dot
txt.Clear()
txt.Dot = dot
This would be a breaking change. But, I'll probably be fine with this change if you also updated all Pixel examples accordingly. Oh, and the tutorial would need to be updated too. If you wanted to do that too, you're welcome, if not, I can do the tutorial.
I'd prefer not to add a new method if we can do the breaking change for Clear
instead (The benefit of not being at 1.0 yet!)
I'll update the examples.
I just got the idea that Clear
can take functional options without breaking backwards compatibility:
// Clear removes all written text from the Text.
func (txt *Text) Clear(options ...func(*Text)) {
txt.prevR = -1
txt.bounds = pixel.Rect{}
txt.tris.SetLen(0)
txt.dirty = true
txt.Dot = txt.Orig
for _, option := range options {
option(txt)
}
}
// SetDot is an option to set the Dot field.
func SetDot(dot pixel.Vec) func(*Text) {
return func(txt *Text) {
txt.Dot = dot
}
}
This could then be used like this:
p.txt.Clear(text.SetDot(p.txt.Dot))
I usually prefer to skip the Set
prefix for options, what do you think about the idea, and naming?
Hm, I think adding the options to Clear
is an overkill. It solves one problem, but generally. Not a fan of that.
And currently, there's no question about the Set
prefix, because Dot
is simply an exported field. That is a deliberate design choice that I'd like to keep.
Oh and one more thing.
p.txt.Clear(text.SetDot(p.txt.Dot))
is the same number of characters as
p.txt.Clear()
p.txt.Dot = p.txt.Orig
So you woudn't save any typing, you'd just squash it in one line.
But that last example wouldn't do the same thing. I just gave an option for the less common case where you want to manually set the Dot field to something that is changed earlier in the Clear method.
Oh, and *text.Text.Dot
and text.Dot
would be distinct identifiers, but it was just a suggestion. I really enjoy using functional options when I need optional configuration of state.
Ah, I was reading it incorrectly. Anyway, still I think it's much clearer and simpler the 3 line way:
dot := txt.Dot
txt.Clear()
txt.Dot = dot
Considering how unusual this is, I think this is sufficient.
*text.Text
would allow us to not having to do this:Adding a
Printf(format string, a ...interface{}) (n int, err error)
method on*text.Text
would allow users of the github.com/faiface/pixel/text package to not directly importfmt
I'd also like us to consider adding a default atlas (
text.BasicAtlas
) initialized by:I believe that doing these changes to the text package would make it more convenient to use. Do you agree @faiface?