Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

led and ledbar widget to visualize binary data #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

persuader72
Copy link

Small widget that show a led representation of a binary number.

image

@andydotxyz
Copy link
Member

Thanks this is very cool.
However I don't think a widget contributed should use text instead of label as a label unless its clear why - you're causing an app to have a mix of text display when that may not be wanted. Would widget.Label work here?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8478408157

Details

  • 0 of 87 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.0%) to 46.784%

Changes Missing Coverage Covered Lines Changed/Added Lines %
widget/led.go 0 87 0.0%
Totals Coverage Status
Change from base Build 8436551209: -1.0%
Covered Lines: 1884
Relevant Lines: 4027

💛 - Coveralls

@persuader72
Copy link
Author

Hi @andydotxyz I made some tests with widget.Label but this widget inherits a lot of settings from theme (like padding for example). That make the text very hard to place under the led icon. (In alternative I would have to make a very bigger led).

This object i more like a gauge so I think the text under it can be considered more like any other graphical item and in this way the coanvas.text give us more freedom to handle it.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to use the canvas.Text in this way it will need to respect the theme for at least the colour, if not the size information as well.

And you should test that it updates when the theme changes too!

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I agree with Andrew about using label instead. I also added a few suggestions inline.

Comment on lines +12 to +16
var (
counter1 int
ledbar1 *xwidget.LedBar
ledbar2 *xwidget.LedBar
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not use global variables please. Use an anonymous function in the main function instead or update the run function to take in pointer parameters for the things you have to modify.

Comment on lines +136 to +140
if (state & b) == b {
w.leds[l-i].(*Led).Set(true)
} else {
w.leds[l-i].(*Led).Set(false)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can pass in the boolean directly instead of branching.

Suggested change
if (state & b) == b {
w.leds[l-i].(*Led).Set(true)
} else {
w.leds[l-i].(*Led).Set(false)
}
w.leds[l-i].(*Led).Set(state & b == b)

Comment on lines +131 to +133

var l int = len(w.texts) - 1
var b int = 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var l int = len(w.texts) - 1
var b int = 1
l := len(w.texts) - 1
b := 1

Comment on lines +69 to +73
if w.state {
return w.OnColor
} else {
return w.offColor
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if w.state {
return w.OnColor
} else {
return w.offColor
}
if w.state {
return w.OnColor
}
return w.offColor

}

func (h *ledRenderer) MinSize() fyne.Size {
log.Println(h.led.Size())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the debug print.


func (h *ledRenderer) MinSize() fyne.Size {
log.Println(h.led.Size())
return fyne.NewSize(h.led.Size().Width, h.led.Size().Height)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if the current size should be the minimum but this is at least simpler than what is there currently:

Suggested change
return fyne.NewSize(h.led.Size().Width, h.led.Size().Height)
return h.led.Size()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants