-
-
Notifications
You must be signed in to change notification settings - Fork 809
Conversation
d2core/d2ui/sprite.go
Outdated
@@ -70,7 +74,7 @@ func (s *Sprite) RenderSegmented(target d2interface.Surface, segmentsX, segments | |||
for x := 0; x < segmentsX; x++ { | |||
idx := x + y*segmentsX + frameOffset*segmentsX*segmentsY | |||
if err := s.animation.SetCurrentFrame(idx); err != nil { | |||
log.Printf("SetCurrentFrame error %e", err) | |||
s.logger.Error("SetCurrentFrame error" + err.Error()) |
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.
For anything that shares a logger instance with some type of manager, we need to alter the log messages to be more descriptive. Currently, this message would print as:
[UI Manager][ERROR] SetCurrentFrame error ...
and that's not at all as useful as
[UI Manager][ERROR] Sprite.RenderSegmented: SetCurrentFrame error ...
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.
There are some issues with this PR that need to be addressed.
First issue -- in many other places where d2util.Logger
instances are added, they are embedded as such:
type foo struct {
*d2util.Logger
}
this allows us to use the logger methods as if they were methods of the struct. This makes the code nicer to look at, but is not a gold standard by any means, and is just my personal preference.
f := &foo{
Logger: d2util.NewLogger()
}
f.Info("example")
However, the precedent is set, and I'd rather we be consistent. If anyone has any objections we can switch to giving them un-exported names in the structs, I don't really care, this is just a matter of consistency.
Second issue -- you added logger instances to widgets in your previous logger PR, why are you not adding them to the widgets you've modified in this PR? Again, this is about consistency.
Third issue -- Anything that shares a logger with some parent class should have more descriptive log messages. This might not need to be addressed, as I think the only things that are doing this are ui/gui widgets, and if these get their own logger instances then it's a moot point. However, if there is anything that is not a ui/gui widget and it doesn;t ave it's own logger instance, then the comments should be more descriptive.
@@ -70,7 +74,7 @@ func (s *Sprite) RenderSegmented(target d2interface.Surface, segmentsX, segments | |||
for x := 0; x < segmentsX; x++ { | |||
idx := x + y*segmentsX + frameOffset*segmentsX*segmentsY | |||
if err := s.animation.SetCurrentFrame(idx); err != nil { | |||
log.Printf("SetCurrentFrame error %e", err) | |||
s.Error("SetCurrentFrame error" + err.Error()) |
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 can be:
s.Errorf("SetCurrentFrame error: %s", err.Error())
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.
anytime we are using a logger method like this:
foo.Info(fmt.Sprintf(fmtMessage, args...))
if should be replaced with the logger format functions:
foo.Infof(fmtMessage, args...)
d2core/d2screen/screen_manager.go
Outdated
@@ -55,7 +53,7 @@ func (sm *ScreenManager) Advance(elapsed float64) error { | |||
} | |||
|
|||
if load.err != nil { | |||
sm.logger.Error(fmt.Sprintf("PROBLEM LOADING THE SCREEN: %v", load.err)) | |||
sm.logger.Errorf("PROBLEM LOADING THE SCREEN: %v", load.err) |
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.
should embed logger instance into screen manager
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.
There's one more logger that needs to be embedded (in the screen manager), other than that this is looking good. I'll merge after you edit.
Hi there,
this is secound part of implementation logger. Now for d2core