ronantakizawa/codereview-bench huggingface dataset:
Repository: fyne-io/fyne
File: internal/preferences.go (Go)

Code before:
func (p *InMemoryPreferences) AddChangeListener(listener func()) {
	p.lock.Lock()
	defer p.lock.Unlock()

	p.changeListeners = append(p.changeListeners, listener)
}

// Bool looks up a boolean value for the key
func (p *InMemoryPreferences) Bool(key string) bool {
	return p.BoolWithFallback(key, false)
}

func (p *InMemoryPreferences) BoolList(key string) []bool {
	return p.BoolListWithFallback(key, []bool{})
}

func (p *InMemoryPreferences) BoolListWithFallback(key string, fallback []bool) []bool {
	value, ok := p.get(key)
	if !ok {
		return fallback
	}
	_, ok1 := value.([]bool)
	if !ok1 {
		return fallback
	}
	return value.([]bool)
}

// BoolWithFallback looks up a boolean value and returns the given fallback if not found
func (p *InMemoryPreferences) BoolWithFallback(key string, fallback bool) bool {
	value, ok := p.get(key)
	if !ok {
		return fallback
	}
	_, ok1 := value.(bool)
	if !ok1 {
		return fallback
	}
	return value.(bool)
}

// ChangeListeners returns the list of listeners registered for this set of preferences.
func (p *InMemoryPreferences) ChangeListeners() []func() {
	return p.changeListeners
}

// Float looks up a float64 value for the key
func (p *InMemoryPreferences) Float(key string) float64 {
	return p.FloatWithFallback(key, 0.0)
}


Review comment (suggestion):
You are doing a second cast here which is unnecessary work. Just save the first parameter of the `_, ok` call above (replacing `_` with `value`).

Code after:
func (p *InMemoryPreferences) AddChangeListener(listener func()) {
	p.lock.Lock()
	defer p.lock.Unlock()

	p.changeListeners = append(p.changeListeners, listener)
}

// Bool looks up a boolean value for the key
func (p *InMemoryPreferences) Bool(key string) bool {
	return p.BoolWithFallback(key, false)
}

func (p *InMemoryPreferences) BoolList(key string) []bool {
	return p.BoolListWithFallback(key, []bool{})
}

func (p *InMemoryPreferences) BoolListWithFallback(key string, fallback []bool) []bool {
	value, ok := p.get(key)
	if !ok {
		return fallback
	}
	valb, ok := value.([]bool)
	if !ok {
		return fallback
	}
	return valb
}

// BoolWithFallback looks up a boolean value and returns the given fallback if not found
func (p *InMemoryPreferences) BoolWithFallback(key string, fallback bool) bool {
	value, ok := p.get(key)
	if !ok {
		return fallback
	}
	valb, ok := value.(bool)
	if !ok {
		return fallback
	}
	return valb
}

// ChangeListeners returns the list of listeners registered for this set of preferences.
func (p *InMemoryPreferences) ChangeListeners() []func() {
	return p.changeListeners
}

// Float looks up a float64 value for the key
func (p *InMemoryPreferences) Float(key string) float64 {
	return p.FloatWithFallback(key, 0.0)
}
Repository: fyne-io/fyne
File: widget/check.go (Go)

Code before:
	c.focused = true

	c.Refresh()
}

// FocusLost is called when the Check has had focus removed.
func (c *Check) FocusLost() {
	c.focused = false

	c.Refresh()
}

// TypedRune receives text input events when the Check is focused.
func (c *Check) TypedRune(r rune) {
	if c.Disabled() {
		return
	}
	if r == ' ' {
		c.SetChecked(!c.Checked)
	}
}

// TypedKey receives key input events when the Check is focused.
func (c *Check) TypedKey(key *fyne.KeyEvent) {}

// SetText sets the text of the Check
func (c *Check) SetText(text string) {
	c.Text = text
	c.Refresh()
}

// Unbind disconnects any configured data source from this Check.
// The current value will remain at the last value of the data source.
//
// Since: 2.0
func (c *Check) Unbind() {
	c.OnChanged = nil
	c.binder.Unbind()
}

func (c *Check) updateFromData(data binding.DataItem) {
	if data == nil {
		return
	}
	boolSource, ok := data.(binding.Bool)
	if !ok {
		return
	}
	val, err := boolSource.Get()
	if err != nil {
		fyne.LogError("Error getting current data value", err)

Review comment (suggestion):
Thanks for this, you would add since in the comment before the function definition, using:
```go
//
// Since: v2.4
```

Code after:
	c.focused = true

	c.Refresh()
}

// FocusLost is called when the Check has had focus removed.
func (c *Check) FocusLost() {
	c.focused = false

	c.Refresh()
}

// TypedRune receives text input events when the Check is focused.
func (c *Check) TypedRune(r rune) {
	if c.Disabled() {
		return
	}
	if r == ' ' {
		c.SetChecked(!c.Checked)
	}
}

// TypedKey receives key input events when the Check is focused.
func (c *Check) TypedKey(key *fyne.KeyEvent) {}

// SetText sets the text of the Check
//
// Since: 2.4
func (c *Check) SetText(text string) {
	c.Text = text
	c.Refresh()
}

// Unbind disconnects any configured data source from this Check.
// The current value will remain at the last value of the data source.
//
// Since: 2.0
func (c *Check) Unbind() {
	c.OnChanged = nil
	c.binder.Unbind()
}

func (c *Check) updateFromData(data binding.DataItem) {
	if data == nil {
		return
	}
	boolSource, ok := data.(binding.Bool)
	if !ok {
		return
	}
	val, err := boolSource.Get()
Repository: fyne-io/fyne
File: widget/button.go (Go)

Code before:
package widget

import (
	"image/color"

	"fyne.io/fyne/v2"
	"fyne.io/fyne/v2/canvas"
	"fyne.io/fyne/v2/driver/desktop"
	col "fyne.io/fyne/v2/internal/color"
	"fyne.io/fyne/v2/internal/widget"
	"fyne.io/fyne/v2/layout"
	"fyne.io/fyne/v2/theme"
)

// ButtonAlign represents the horizontal alignment of a button.
type ButtonAlign int

// ButtonIconPlacement represents the ordering of icon & text within a button.
type ButtonIconPlacement int

// ButtonImportance represents how prominent the button should appear
//
// Since: 1.4
// Deprecated: Use widget.Importance instead
type ButtonImportance Importance

// ButtonStyle determines the behaviour and rendering of a button.
type ButtonStyle int

const (
	// ButtonAlignCenter aligns the icon and the text centrally.
	ButtonAlignCenter ButtonAlign = iota
	// ButtonAlignLeading aligns the icon and the text with the leading edge.
	ButtonAlignLeading
	// ButtonAlignTrailing aligns the icon and the text with the trailing edge.
	ButtonAlignTrailing
)

const (
	// ButtonIconLeadingText aligns the icon on the leading edge of the text.
	ButtonIconLeadingText ButtonIconPlacement = iota
	// ButtonIconTrailingText aligns the icon on the trailing edge of the text.
	ButtonIconTrailingText
)

var _ fyne.Focusable = (*Button)(nil)

// Button widget has a text label and triggers an event func when clicked
type Button struct {
	DisableableWidget

Review comment (suggestion):
I believe that you need to have the deprecation as a new paragraph.
```suggestion
//
// Deprecated: Use widget.Importance instead
```

Code after:
package widget

import (
	"image/color"

	"fyne.io/fyne/v2"
	"fyne.io/fyne/v2/canvas"
	"fyne.io/fyne/v2/driver/desktop"
	col "fyne.io/fyne/v2/internal/color"
	"fyne.io/fyne/v2/internal/widget"
	"fyne.io/fyne/v2/layout"
	"fyne.io/fyne/v2/theme"
)

// ButtonAlign represents the horizontal alignment of a button.
type ButtonAlign int

// ButtonIconPlacement represents the ordering of icon & text within a button.
type ButtonIconPlacement int

// ButtonImportance represents how prominent the button should appear
//
// Since: 1.4
//
// Deprecated: Use widget.Importance instead
type ButtonImportance = Importance

// ButtonStyle determines the behaviour and rendering of a button.
type ButtonStyle int

const (
	// ButtonAlignCenter aligns the icon and the text centrally.
	ButtonAlignCenter ButtonAlign = iota
	// ButtonAlignLeading aligns the icon and the text with the leading edge.
	ButtonAlignLeading
	// ButtonAlignTrailing aligns the icon and the text with the trailing edge.
	ButtonAlignTrailing
)

const (
	// ButtonIconLeadingText aligns the icon on the leading edge of the text.
	ButtonIconLeadingText ButtonIconPlacement = iota
	// ButtonIconTrailingText aligns the icon on the trailing edge of the text.
	ButtonIconTrailingText
)

var _ fyne.Focusable = (*Button)(nil)

// Button widget has a text label and triggers an event func when clicked
type Button struct {
Repository: fyne-io/fyne
File: widget/importance.go (Go)

Code before:
package widget

type Importance int

const (
	// MediumImportance applies a standard appearance.
	MediumImportance Importance = iota
	// HighImportance applies a prominent appearance.
	HighImportance
	// LowImportance applies a subtle appearance.
	LowImportance

	// DangerImportance applies an error theme to the widget.
	//
	// Since 2.3
	DangerImportance
	// WarningImportance applies a warning theme to the widget.
	//
	// Since 2.3
	WarningImportance

	// SuccessImportance applies a success theme to the widget.
	SuccessImportance
)


Review comment (suggestion):
Please add a comment and a `Since: 2.4` line to this type.

Code after:
package widget

// Importance represents how prominent the widget should appear
//
// Since: 2.4
type Importance int

const (
	// MediumImportance applies a standard appearance.
	MediumImportance Importance = iota
	// HighImportance applies a prominent appearance.
	HighImportance
	// LowImportance applies a subtle appearance.
	LowImportance

	// DangerImportance applies an error theme to the widget.
	//
	// Since 2.3
	DangerImportance
	// WarningImportance applies a warning theme to the widget.
	//
	// Since 2.3
	WarningImportance

	// SuccessImportance applies a success theme to the widget.
	//
	// Since 2.4
	SuccessImportance
)
Repository: fyne-io/fyne
File: widget/importance.go (Go)

Code before:
package widget

type Importance int

const (
	// MediumImportance applies a standard appearance.
	MediumImportance Importance = iota
	// HighImportance applies a prominent appearance.
	HighImportance
	// LowImportance applies a subtle appearance.
	LowImportance

	// DangerImportance applies an error theme to the widget.
	//
	// Since 2.3
	DangerImportance
	// WarningImportance applies a warning theme to the widget.
	//
	// Since 2.3
	WarningImportance

	// SuccessImportance applies a success theme to the widget.
	SuccessImportance
)


Review comment (suggestion):
Hmm. I'm not sure about this as an importance. At least `importance` doesn't seem to be the right word for it in my opinion. Can we leave that out for now and focus on adding importance support to label? If you wish to add this, we can do that in a follow-up PR and make this PR go a bit quicker.

Code after:
// Importance represents how prominent the widget should appear
//
// Since: 2.4
type Importance int

const (
	// MediumImportance applies a standard appearance.
	MediumImportance Importance = iota
	// HighImportance applies a prominent appearance.
	HighImportance
	// LowImportance applies a subtle appearance.
	LowImportance

	// DangerImportance applies an error theme to the widget.
	//
	// Since 2.3
	DangerImportance
	// WarningImportance applies a warning theme to the widget.
	//
	// Since 2.3
	WarningImportance

	// SuccessImportance applies a success theme to the widget.
	//
	// Since 2.4
	SuccessImportance
)
Repository: fyne-io/fyne
File: widget/button_test.go (Go)

Code before:
	test.AssertImageMatches(t, "button/theme_initial.png", w.Canvas().Capture())

	test.WithTestTheme(t, func() {
		b.Resize(b.MinSize())
		b.Refresh()
		time.Sleep(100 * time.Millisecond)
		test.AssertImageMatches(t, "button/theme_changed.png", w.Canvas().Capture())
	})
}

func TestButtonCompatImportance(t *testing.T) {
	test.NewApp()
	defer test.NewApp()
	test.ApplyTheme(t, theme.LightTheme())

	//test backward compatibility of widget.Importance
	var imp widget.ButtonImportance = widget.HighImportance

	btn := widget.NewButton("test", func() {})
	btn.Importance = imp
	btn.Refresh()

	w := test.NewWindow(btn)
	defer w.Close()

	test.AssertImageMatches(t, "button/compat_importance.png", w.Canvas().Capture())
}


Review comment (suggestion):
I don't think you need any image for this if you just want to make sure that it compiles (please also start comments with a space and then a capital letter):
```suggestion
	// Test backward compatibility of widget.Importance
	var imp widget.ButtonImportance = widget.HighImportance

	btn := widget.NewButton("test", func() {})
	btn.Importance = imp
```
Remember to delete the corresponding image as well.

Code after:
	test.AssertImageMatches(t, "button/theme_initial.png", w.Canvas().Capture())

	test.WithTestTheme(t, func() {
		b.Resize(b.MinSize())
		b.Refresh()
		time.Sleep(100 * time.Millisecond)
		test.AssertImageMatches(t, "button/theme_changed.png", w.Canvas().Capture())
	})
}

func TestButtonCompatImportance(t *testing.T) {
	// Test backward compatibility of widget.Importance
	var imp widget.ButtonImportance = widget.HighImportance

	btn := widget.NewButton("test", func() {})
	btn.Importance = imp
}

func TestButtonSuccess(t *testing.T) {
	test.NewApp()
	defer test.NewApp()
	test.ApplyTheme(t, theme.LightTheme())

	b := widget.NewButtonWithIcon("Test", theme.HomeIcon(), func() {})
	w := test.NewWindow(b)
	defer w.Close()

	b.Importance = widget.SuccessImportance
	b.Refresh()

	test.AssertImageMatches(t, "button/success_importance.png", w.Canvas().Capture())
}
Repository: fyne-io/fyne
File: widget/entry.go (Go)

Code before:

	e.placeholderProvider().Segments[0].(*TextSegment).Text = text
	e.placeholder.updateRowBounds()
	e.placeholderProvider().Refresh()
}

// SetText manually sets the text of the Entry to the given text value.
func (e *Entry) SetText(text string) {
	e.updateTextAndRefresh(text)

	e.updateCursorAndSelection()
}

// Appends the text to the end of the entry
//
// Since: 2.4
func (e *Entry) Append(text string) {
	e.propertyLock.Lock()
	provider := e.textProvider()
	provider.insertAt(provider.len(), text)
	changed := e.updateText(provider.String())
	e.propertyLock.Unlock()

	e.Validate()
	if changed && e.OnChanged != nil {
		e.OnChanged(provider.String())
	}
	e.Refresh()
}

// Tapped is called when this entry has been tapped. We update the cursor position in
// device-specific callbacks (MouseDown() and TouchDown()).
//
// Implements: fyne.Tappable
func (e *Entry) Tapped(ev *fyne.PointEvent) {
	if fyne.CurrentDevice().IsMobile() && e.selecting {
		e.selecting = false
	}
}

// TappedSecondary is called when right or alternative tap is invoked.
//
// Opens the PopUpMenu with `Paste` item to paste text from the clipboard.
//
// Implements: fyne.SecondaryTappable
func (e *Entry) TappedSecondary(pe *fyne.PointEvent) {
	if e.Disabled() && e.Password {
		return // no popup options for a disabled concealed field
	}

	e.requestFocus()

Review comment (suggestion):
You should avoid calling `provider.String()` multiple times. If you look at the code you will see that it allocates an entirely new string each time. You probably want to save the string to a variable. Have a look at `TypedRune` to see how it is handled there.

Code after:

	e.placeholderProvider().Segments[0].(*TextSegment).Text = text
	e.placeholder.updateRowBounds()
	e.placeholderProvider().Refresh()
}

// SetText manually sets the text of the Entry to the given text value.
func (e *Entry) SetText(text string) {
	e.updateTextAndRefresh(text)

	e.updateCursorAndSelection()
}

// Appends the text to the end of the entry
//
// Since: 2.4
func (e *Entry) Append(text string) {
	e.propertyLock.Lock()
	provider := e.textProvider()
	provider.insertAt(provider.len(), text)
	content := provider.String()
	changed := e.updateText(content)
	e.propertyLock.Unlock()

	if changed {
		e.Validate()
		if e.OnChanged != nil {
			e.OnChanged(content)
		}
	}
	e.Refresh()
}

// Tapped is called when this entry has been tapped. We update the cursor position in
// device-specific callbacks (MouseDown() and TouchDown()).
//
// Implements: fyne.Tappable
func (e *Entry) Tapped(ev *fyne.PointEvent) {
	if fyne.CurrentDevice().IsMobile() && e.selecting {
		e.selecting = false
	}
}

// TappedSecondary is called when right or alternative tap is invoked.
//
// Opens the PopUpMenu with `Paste` item to paste text from the clipboard.
//
// Implements: fyne.SecondaryTappable
func (e *Entry) TappedSecondary(pe *fyne.PointEvent) {
	if e.Disabled() && e.Password {
		return // no popup options for a disabled concealed field
Repository: fyne-io/fyne
File: widget/entry.go (Go)

Code before:
		e.propertyLock.Lock()
		if e.MultiLine {
			e.CursorRow = 0
		}
		e.CursorColumn = 0
		e.propertyLock.Unlock()
	case fyne.KeyPageDown:
		e.propertyLock.Lock()
		if e.MultiLine {
			e.CursorRow = provider.rows() - 1
			e.CursorColumn = provider.rowLength(e.CursorRow)
		} else {
			e.CursorColumn = provider.len()
		}
		e.propertyLock.Unlock()
	default:
		return
	}

	e.propertyLock.Lock()
	changed := e.updateText(provider.String())
	if e.CursorRow == e.selectRow && e.CursorColumn == e.selectColumn {
		e.selecting = false
	}
	e.propertyLock.Unlock()
	e.Validate()
	if changed && e.OnChanged != nil {
		e.OnChanged(provider.String())
	}
	e.Refresh()
}

func (e *Entry) typedKeyUp(provider *RichText) {
	e.propertyLock.Lock()

	if e.CursorRow > 0 {
		e.CursorRow--
	} else {
		e.CursorColumn = 0
	}

	rowLength := provider.rowLength(e.CursorRow)
	if e.CursorColumn > rowLength {
		e.CursorColumn = rowLength
	}
	e.propertyLock.Unlock()
}

func (e *Entry) typedKeyDown(provider *RichText) {
	e.propertyLock.Lock()
	rowLength := provider.rowLength(e.CursorRow)

Review comment (performance):
This leads to unwanted side effects. Pressing tab inside a multiline entry calls validation twice but we also validate the text even in all cases where we know that nothing has changed (i.e. moving the cursor using tab keys but also pressing keys like home, page up, end etc.). I don't think doing validation that often will be good if users have slow validators.

Code after:
		e.CursorColumn = 0
		e.propertyLock.Unlock()
	case fyne.KeyPageUp:
		e.propertyLock.Lock()
		if e.MultiLine {
			e.CursorRow = 0
		}
		e.CursorColumn = 0
		e.propertyLock.Unlock()
	case fyne.KeyPageDown:
		e.propertyLock.Lock()
		if e.MultiLine {
			e.CursorRow = provider.rows() - 1
			e.CursorColumn = provider.rowLength(e.CursorRow)
		} else {
			e.CursorColumn = provider.len()
		}
		e.propertyLock.Unlock()
	default:
		return
	}

	e.propertyLock.Lock()
	content := provider.String()
	changed := e.updateText(content)
	if e.CursorRow == e.selectRow && e.CursorColumn == e.selectColumn {
		e.selecting = false
	}
	e.propertyLock.Unlock()
	if changed {
		e.Validate()
		if e.OnChanged != nil {
			e.OnChanged(content)
		}
	}
	e.Refresh()
}

func (e *Entry) typedKeyUp(provider *RichText) {
	e.propertyLock.Lock()

	if e.CursorRow > 0 {
		e.CursorRow--
	} else {
		e.CursorColumn = 0
	}

	rowLength := provider.rowLength(e.CursorRow)
	if e.CursorColumn > rowLength {
		e.CursorColumn = rowLength
	}
Repository: fyne-io/fyne
File: widget/entry.go (Go)

Code before:
	}

	e.copyToClipboard(clipboard)
	e.setFieldsAndRefresh(e.eraseSelection)
}

// eraseSelection removes the current selected region and moves the cursor
func (e *Entry) eraseSelection() {
	if e.Disabled() {
		return
	}

	provider := e.textProvider()
	posA, posB := e.selection()

	if posA == posB {
		return
	}

	provider.deleteFromTo(posA, posB)
	e.CursorRow, e.CursorColumn = e.rowColFromTextPos(posA)
	e.selectRow, e.selectColumn = e.CursorRow, e.CursorColumn
	e.selecting = false
	changed := e.updateText(provider.String())

	e.Validate()
	if changed && e.OnChanged != nil {
		e.OnChanged(provider.String())
	}
}

func (e *Entry) getRowCol(p fyne.Position) (int, int) {
	e.propertyLock.RLock()
	defer e.propertyLock.RUnlock()

	rowHeight := e.textProvider().charMinSize(e.Password, e.TextStyle).Height
	row := int(math.Floor(float64(p.Y+e.scroll.Offset.Y-theme.LineSpacing()) / float64(rowHeight)))
	col := 0
	if row < 0 {
		row = 0
	} else if row >= e.textProvider().rows() {
		row = e.textProvider().rows() - 1
		col = e.textProvider().rowLength(row)
	} else {
		col = e.cursorColAt(e.textProvider().row(row), p.Add(e.scroll.Offset))
	}

	return row, col
}

// pasteFromClipboard inserts text from the clipboard content,

Review comment (suggestion):
This change also leads to verifying twice (potentially running `OnChanged` twice as well, I would assume) when selecting something and then pressing another key using `TypedKey` or `TypedRune` to replace the text.

Code after:

// copyToClipboard copies the current selection to a given clipboard and then removes the selected text.
// This does nothing if it is a concealed entry.
func (e *Entry) cutToClipboard(clipboard fyne.Clipboard) {
	if !e.selecting || e.Password {
		return
	}

	e.copyToClipboard(clipboard)
	e.setFieldsAndRefresh(e.eraseSelection)
	e.propertyLock.Lock()
	if e.OnChanged != nil {
		e.OnChanged(e.Text)
	}
	e.propertyLock.Unlock()
	e.Validate()
}

// eraseSelection removes the current selected region and moves the cursor
func (e *Entry) eraseSelection() {
	if e.Disabled() {
		return
	}

	provider := e.textProvider()
	posA, posB := e.selection()

	if posA == posB {
		return
	}

	provider.deleteFromTo(posA, posB)
	e.CursorRow, e.CursorColumn = e.rowColFromTextPos(posA)
	e.selectRow, e.selectColumn = e.CursorRow, e.CursorColumn
	e.selecting = false
	e.updateText(provider.String())
}

func (e *Entry) getRowCol(p fyne.Position) (int, int) {
	e.propertyLock.RLock()
	defer e.propertyLock.RUnlock()

	rowHeight := e.textProvider().charMinSize(e.Password, e.TextStyle).Height
	row := int(math.Floor(float64(p.Y+e.scroll.Offset.Y-theme.LineSpacing()) / float64(rowHeight)))
	col := 0
	if row < 0 {
		row = 0
	} else if row >= e.textProvider().rows() {
		row = e.textProvider().rows() - 1
		col = e.textProvider().rowLength(row)
	} else {