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

Code before:
	}
	cb := e.OnChanged
	e.propertyLock.Unlock()
	if changed {
		e.validate()
		if cb != nil {
			cb(content)
		}
	}
	e.Refresh()
}

// Undo un-does the last modifying user-action.
//
// Since: 2.5
func (e *Entry) Undo() {
	e.propertyLock.Lock()
	newText, action := e.undoStack.Undo(e.Text)
	e.propertyLock.Unlock()
	modify, ok := action.(*entryModifyAction)
	if !ok {
		return
	}
	pos := modify.Position
	if modify.Delete {
		pos += len([]rune(modify.Text))
	}
	e.propertyLock.Lock()
	e.updateText(newText, false)
	e.CursorRow, e.CursorColumn = e.rowColFromTextPos(pos)
	e.propertyLock.Unlock()
	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) {

Review comment (performance):
Here and anywhere else you want the length of a string in runes, it should be more efficient to use `utf8.RuneCountInString(modify.Text)` which avoids an allocation

Code after:
		e.selecting = false
	}
	cb := e.OnChanged
	e.propertyLock.Unlock()
	if changed {
		e.validate()
		if cb != nil {
			cb(content)
		}
	}
	e.Refresh()
}

// Undo un-does the last modifying user-action.
//
// Since: 2.5
func (e *Entry) Undo() {
	e.propertyLock.Lock()
	newText, action := e.undoStack.Undo(e.Text)
	e.propertyLock.Unlock()
	modify, ok := action.(*entryModifyAction)
	if !ok {
		return
	}
	pos := modify.Position
	if modify.Delete {
		pos += len(modify.Text)
	}
	e.propertyLock.Lock()
	e.updateText(newText, false)
	e.CursorRow, e.CursorColumn = e.rowColFromTextPos(pos)
	e.propertyLock.Unlock()
	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()
}
Repository: fyne-io/fyne
File: widget/entry.go (Go)

Code before:
// It represents the insertion/deletion of a single string at a
// position (e.g. "Hello" => "Hello, world", or "Hello" => "He").
type entryModifyAction struct {
	// Delete is true if this action deletes Text, and false if it inserts Text
	Delete bool
	// Position represents the start position of Text
	Position int
	// Text is the text that is inserted or deleted at Position
	Text string
}

func (i *entryModifyAction) Undo(s string) string {
	if i.Delete {
		return i.add(s)
	} else {
		return i.sub(s)
	}
}

func (i *entryModifyAction) Redo(s string) string {
	if i.Delete {
		return i.sub(s)
	} else {
		return i.add(s)
	}
}

// Inserts Text
func (i *entryModifyAction) add(s string) string {
	return s[:i.Position] + i.Text + s[i.Position:]
}

// Deletes Text
func (i *entryModifyAction) sub(s string) string {
	return s[:i.Position] + s[i.Position+len(i.Text):]
}

func (i *entryModifyAction) TryMerge(other entryMergeableUndoAction) bool {
	if other, ok := other.(*entryModifyAction); ok {
		// Don't merge two different types of modifyAction
		if i.Delete != other.Delete {
			return false
		}

		// Don't merge two separate words
		wordSeparators := func(s string) (num int, onlyWordSeparators bool) {
			onlyWordSeparators = true
			for _, r := range s {
				if isWordSeparator(r) {
					num++
					onlyWordSeparators = false

Review comment (refactor):
We have casting to strings that result in allocations here and in `sub` now. Would refactoring these to receive and return []rune result in fewer castings still?

Code after:
	// Delete is true if this action deletes Text, and false if it inserts Text
	Delete bool
	// Position represents the start position of Text
	Position int
	// Text is the text that is inserted or deleted at Position
	Text []rune
}

func (i *entryModifyAction) Undo(s string) string {
	if i.Delete {
		return i.add(s)
	} else {
		return i.sub(s)
	}
}

func (i *entryModifyAction) Redo(s string) string {
	if i.Delete {
		return i.sub(s)
	} else {
		return i.add(s)
	}
}

// Inserts Text
func (i *entryModifyAction) add(s string) string {
	runes := []rune(s)
	return string(runes[:i.Position]) + string(i.Text) + string(runes[i.Position:])
}

// Deletes Text
func (i *entryModifyAction) sub(s string) string {
	runes := []rune(s)
	return string(runes[:i.Position]) + string(runes[i.Position+len(i.Text):])
}

func (i *entryModifyAction) TryMerge(other entryMergeableUndoAction) bool {
	if other, ok := other.(*entryModifyAction); ok {
		// Don't merge two different types of modifyAction
		if i.Delete != other.Delete {
			return false
		}

		// Don't merge two separate words
		wordSeparators := func(s []rune) (num int, onlyWordSeparators bool) {
			onlyWordSeparators = true
			for _, r := range s {
				if isWordSeparator(r) {
					num++
					onlyWordSeparators = false
				}
Repository: fyne-io/fyne
File: lang/lang_test.go (Go)

Code before:
package lang

import (
	"testing"

	"fyne.io/fyne/v2"
	"github.com/nicksnyder/go-i18n/v2/i18n"
	"github.com/stretchr/testify/assert"
)

func TestAddTranslations(t *testing.T) {
	setupLang("en")
	err := AddTranslations(fyne.NewStaticResource("en.json", []byte(`{
  "Test": "Match"
}`)))
	assert.Nil(t, err)
	assert.Equal(t, "Match", L("Test"))

	err = AddTranslationsForLocale([]byte(`{
  "Test2": "Match2"
}`), "fr")
	setupLang("fr")
	assert.Nil(t, err)
	assert.Equal(t, "Match2", L("Test2"))
}

func TestLocalize_Default(t *testing.T) {
	setupLang("en")
	err := AddTranslationsForLocale([]byte(`{
  "Test": "Match"
}`), "en")
	assert.Nil(t, err)

	err = AddTranslationsForLocale([]byte(`{
  "Test": "Match2"
}`), "de")
	assert.Nil(t, err)

	fr := i18n.NewLocalizer(bundle, "fr")
	ret, err := fr.Localize(&i18n.LocalizeConfig{
		DefaultMessage: &i18n.Message{
			ID: "Test",
		},
	})

	assert.NotNil(t, err) // we are falling back
	assert.Equal(t, "Match", ret)
}

func TestLocalize_Fallback(t *testing.T) {
	assert.Equal(t, "Missing", L("Missing"))

Review comment (bug):
Strangely, this test passes even with the code change commented out, and I played around with various permutations of the test code here and I couldn't get it to fail for the old code and pass for the new code, so I'm not sure what it's actually testing

Code after:
package lang

import (
	"testing"

	"fyne.io/fyne/v2"
	"github.com/stretchr/testify/assert"
)

func TestAddTranslations(t *testing.T) {
	setupLang("en")
	err := AddTranslations(fyne.NewStaticResource("en.json", []byte(`{
  "Test": "Match"
}`)))
	assert.Nil(t, err)
	assert.Equal(t, "Match", L("Test"))

	err = AddTranslationsForLocale([]byte(`{
  "Test2": "Match2"
}`), "fr")
	setupLang("fr")
	assert.Nil(t, err)
	assert.Equal(t, "Match2", L("Test2"))
}

func TestLocalize_Default(t *testing.T) {
	fallback := closestSupportedLocale([]string{"xx"})
	assert.Equal(t, fyne.Locale("en"), fallback[0:2])
}

func TestLocalize_Fallback(t *testing.T) {
	assert.Equal(t, "Missing", L("Missing"))
}

func TestLocalize_Struct(t *testing.T) {
	type data struct {
		Str string
	}

	assert.Equal(t, "Hello World!", L("Hello {{.Str}}!", data{Str: "World"}))
}

func TestLocalize_Map(t *testing.T) {
	assert.Equal(t, "Hello World!", L("Hello {{.Str}}!", map[string]string{"Str": "World"}))
}

func TestLocalizePlural_Fallback(t *testing.T) {
	_ = AddTranslations(fyne.NewStaticResource("en.json", []byte(`{
  "Apple": {
    "one": "Apple",
    "other": "Apples"
Repository: fyne-io/fyne
File: internal/repository/file.go (Go)

Code before:
//
// Since: 2.0
func (r *FileRepository) Delete(u fyne.URI) error {
	return os.Remove(u.Path())
}

// Parent implements repository.HierarchicalRepository.Parent
//
// Since: 2.0
func (r *FileRepository) Parent(u fyne.URI) (fyne.URI, error) {
	s := u.String()

	// trim trailing slash
	s = strings.TrimSuffix(s, "/")

	// trim the scheme
	s = strings.TrimPrefix(s, fileSchemePrefix)

	// Completely empty URI or only root component
	if s == "" || s == "/" || (len(s) == 2 && s[1] == ':') {
		return nil, repository.ErrURIRoot
	}

	child := filepath.Base(s)
	parent := s[:len(s)-len(child)]
	if parent[len(parent)-1] != '/' {
		parent += "/"
	}

	// only root is its own parent
	if filepath.Clean(parent) == filepath.Clean(s) {
		return nil, repository.ErrURIRoot
	}

	return storage.NewFileURI(parent), nil
}

// Child implements repository.HierarchicalRepository.Child
//
// Since: 2.0
func (r *FileRepository) Child(u fyne.URI, component string) (fyne.URI, error) {
	newURI := u.Scheme() + "://" + u.Authority()
	newURI += path.Join(u.Path(), component)

	// stick the query and fragment back on the end
	if query := u.Query(); len(query) > 0 {
		newURI += "?" + query
	}
	if fragment := u.Fragment(); len(fragment) > 0 {
		newURI += "#" + fragment
	}

Review comment (suggestion):
Already a problem before this change obviously but will this not do `parent[-1]` if parent is empty? Maybe that can't ever happen but I just got a bit worried when seeing it now :)

Code after:
//
// Since: 2.0
func (r *FileRepository) Delete(u fyne.URI) error {
	return os.Remove(u.Path())
}

// Parent implements repository.HierarchicalRepository.Parent
//
// Since: 2.0
func (r *FileRepository) Parent(u fyne.URI) (fyne.URI, error) {
	s := u.String()

	// trim trailing slash
	s = strings.TrimSuffix(s, "/")

	// trim the scheme
	s = strings.TrimPrefix(s, fileSchemePrefix)

	// Completely empty URI or only root component
	if s == "" || s == "/" || (len(s) == 2 && s[1] == ':') {
		return nil, repository.ErrURIRoot
	}

	child := filepath.Base(s)
	parent := s[:len(s)-len(child)] // avoid filepath.Dir as it follows platform rules
	if parent == "" || parent[len(parent)-1] != '/' {
		parent += "/"
	}

	// only root is its own parent
	if filepath.Clean(parent) == filepath.Clean(s) {
		return nil, repository.ErrURIRoot
	}

	return storage.NewFileURI(parent), nil
}

// Child implements repository.HierarchicalRepository.Child
//
// Since: 2.0
func (r *FileRepository) Child(u fyne.URI, component string) (fyne.URI, error) {
	newURI := u.Scheme() + "://" + u.Authority()
	newURI += path.Join(u.Path(), component)

	// stick the query and fragment back on the end
	if query := u.Query(); len(query) > 0 {
		newURI += "?" + query
	}
	if fragment := u.Fragment(); len(fragment) > 0 {
		newURI += "#" + fragment
	}
Repository: fyne-io/fyne
File: internal/repository/file.go (Go)

Code before:
// Delete implements repository.WritableRepository.Delete
//
// Since: 2.0
func (r *FileRepository) Delete(u fyne.URI) error {
	return os.Remove(u.Path())
}

// Parent implements repository.HierarchicalRepository.Parent
//
// Since: 2.0
func (r *FileRepository) Parent(u fyne.URI) (fyne.URI, error) {
	s := u.String()

	// trim trailing slash
	s = strings.TrimSuffix(s, "/")

	// trim the scheme
	s = strings.TrimPrefix(s, fileSchemePrefix)

	// Completely empty URI or only root component
	if s == "" || s == "/" || (len(s) == 2 && s[1] == ':') {
		return nil, repository.ErrURIRoot
	}

	child := filepath.Base(s)
	parent := s[:len(s)-len(child)]
	if parent[len(parent)-1] != '/' {
		parent += "/"
	}

	// only root is its own parent
	if filepath.Clean(parent) == filepath.Clean(s) {
		return nil, repository.ErrURIRoot
	}

	return storage.NewFileURI(parent), nil
}

// Child implements repository.HierarchicalRepository.Child
//
// Since: 2.0
func (r *FileRepository) Child(u fyne.URI, component string) (fyne.URI, error) {
	newURI := u.Scheme() + "://" + u.Authority()
	newURI += path.Join(u.Path(), component)

	// stick the query and fragment back on the end
	if query := u.Query(); len(query) > 0 {
		newURI += "?" + query
	}
	if fragment := u.Fragment(); len(fragment) > 0 {
		newURI += "#" + fragment

Review comment (bug):
Was there something wrong with `filepath.Dir`? If so, this might warrant a comment as to why we are hard coding the logic.

Code after:
// Delete implements repository.WritableRepository.Delete
//
// Since: 2.0
func (r *FileRepository) Delete(u fyne.URI) error {
	return os.Remove(u.Path())
}

// Parent implements repository.HierarchicalRepository.Parent
//
// Since: 2.0
func (r *FileRepository) Parent(u fyne.URI) (fyne.URI, error) {
	s := u.String()

	// trim trailing slash
	s = strings.TrimSuffix(s, "/")

	// trim the scheme
	s = strings.TrimPrefix(s, fileSchemePrefix)

	// Completely empty URI or only root component
	if s == "" || s == "/" || (len(s) == 2 && s[1] == ':') {
		return nil, repository.ErrURIRoot
	}

	child := filepath.Base(s)
	parent := s[:len(s)-len(child)] // avoid filepath.Dir as it follows platform rules
	if parent == "" || parent[len(parent)-1] != '/' {
		parent += "/"
	}

	// only root is its own parent
	if filepath.Clean(parent) == filepath.Clean(s) {
		return nil, repository.ErrURIRoot
	}

	return storage.NewFileURI(parent), nil
}

// Child implements repository.HierarchicalRepository.Child
//
// Since: 2.0
func (r *FileRepository) Child(u fyne.URI, component string) (fyne.URI, error) {
	newURI := u.Scheme() + "://" + u.Authority()
	newURI += path.Join(u.Path(), component)

	// stick the query and fragment back on the end
	if query := u.Query(); len(query) > 0 {
		newURI += "?" + query
	}
	if fragment := u.Fragment(); len(fragment) > 0 {
		newURI += "#" + fragment
Repository: fyne-io/fyne
File: canvas/canvas.go (Go)

Code before:
	"fyne.io/fyne/v2/internal/svg"
)

// Refresh instructs the containing canvas to refresh the specified obj.
func Refresh(obj fyne.CanvasObject) {
	app := fyne.CurrentApp()
	if app == nil || app.Driver() == nil {
		return
	}

	c := app.Driver().CanvasForObject(obj)
	if c != nil {
		c.Refresh(obj)
	}
}

// RecolorSVG takes a fyne.Resource containing SVG content, and returns
// the raw SVG content, re-colorized to be monochrome with the given color.
// The content can be assigned to a new fyne.StaticResource with an appropriate name
// to be used in a widget.Button, canvas.Image, etc.
//
// If an error occurs, the returned content will be the content of the input Resource,
// and a non-nil error is returned.
//
// Since: 2.6
func RecolorSVG(resource fyne.Resource, color color.Color) ([]byte, error) {
	return svg.ColorizeError(resource.Content(), color)
}

// repaint instructs the containing canvas to redraw, even if nothing changed.
func repaint(obj fyne.CanvasObject) {
	app := fyne.CurrentApp()
	if app == nil || app.Driver() == nil {
		return
	}

	c := app.Driver().CanvasForObject(obj)
	if c != nil {
		if paint, ok := c.(interface{ SetDirty() }); ok {
			paint.SetDirty()
		}
	}
}


Review comment (performance):
It feels like an impedance mismatch to take in a Resource but return a []byte...

If we are expecting people to create an appropriate name for a resource we probably need to be more specific that it should contain the colour name to avoid cache collisions - can we solve this problem for them?

I think it may need to be []byte in and []byte out because this is used internally to ThemedResource handling, so being in canvas package probably implies the lower level API.

Code after:
	"fyne.io/fyne/v2/internal/svg"
)

// Refresh instructs the containing canvas to refresh the specified obj.
func Refresh(obj fyne.CanvasObject) {
	app := fyne.CurrentApp()
	if app == nil || app.Driver() == nil {
		return
	}

	c := app.Driver().CanvasForObject(obj)
	if c != nil {
		c.Refresh(obj)
	}
}

// RecolorSVG takes a []byte containing SVG content, and returns
// new SVG content, re-colorized to be monochrome with the given color.
// The content can be assigned to a new fyne.StaticResource with an appropriate name
// to be used in a widget.Button, canvas.Image, etc.
//
// If an error occurs, the returned content will be the original un-modified content,
// and a non-nil error is returned.
//
// Since: 2.6
func RecolorSVG(svgContent []byte, color color.Color) ([]byte, error) {
	return svg.Colorize(svgContent, color)
}

// repaint instructs the containing canvas to redraw, even if nothing changed.
func repaint(obj fyne.CanvasObject) {
	app := fyne.CurrentApp()
	if app == nil || app.Driver() == nil {
		return
	}

	c := app.Driver().CanvasForObject(obj)
	if c != nil {
		if paint, ok := c.(interface{ SetDirty() }); ok {
			paint.SetDirty()
		}
	}
}
Repository: fyne-io/fyne
File: internal/driver/glfw/window_desktop.go (Go)

Code before:

			if x > xOff || y > yOff {
				continue
			}
			if videoMode := monitor.GetVideoMode(); x+videoMode.Width <= xOff || y+videoMode.Height <= yOff {
				continue
			}

			return monitor
		}
	}

	// try built-in function to detect monitor if above logic didn't succeed
	// if it doesn't work then return primary monitor as default
	monitor := w.viewport.GetMonitor()
	if monitor == nil {
		monitor = glfw.GetPrimaryMonitor()
	}
	return monitor
}

func (w *window) detectScale() float32 {
	// check if DPI detection is disabled
	env := os.Getenv(disableDPIDetectionEnvKey)
	switch env {
	case "True", "TRUE", "true", "T", "t", "1":
		return 1
	}

	if build.IsWayland { // Wayland controls scale through content scaling
		return 1
	}
	monitor := w.getMonitorForWindow()
	if monitor == nil {
		return 1
	}

	widthMm, heightMm := monitor.GetPhysicalSize()
	if runtime.GOOS == "linux" && widthMm == 60 && heightMm == 60 { // Steam Deck incorrectly reports 6cm square!
		return 1
	}
	widthPx := monitor.GetVideoMode().Width

	return calculateDetectedScale(widthMm, widthPx)
}

func (w *window) moved(_ *glfw.Window, x, y int) {
	w.processMoved(x, y)
}

func (w *window) resized(_ *glfw.Window, width, height int) {

Review comment (suggestion):
You might want to use `strings.EqualFold` instead of hardcoding values. That should allow the word "true" no matter how it is capitalised.

Code after:
			x, y := monitor.GetPos()

			if x > xOff || y > yOff {
				continue
			}
			if videoMode := monitor.GetVideoMode(); x+videoMode.Width <= xOff || y+videoMode.Height <= yOff {
				continue
			}

			return monitor
		}
	}

	// try built-in function to detect monitor if above logic didn't succeed
	// if it doesn't work then return primary monitor as default
	monitor := w.viewport.GetMonitor()
	if monitor == nil {
		monitor = glfw.GetPrimaryMonitor()
	}
	return monitor
}

func (w *window) detectScale() float32 {
	if build.IsWayland { // Wayland controls scale through content scaling
		return 1
	}

	// check if DPI detection is disabled
	env := os.Getenv(disableDPIDetectionEnvKey)
	if strings.EqualFold(env, "true") || strings.EqualFold(env, "t") || env == "1" {
		return 1
	}

	monitor := w.getMonitorForWindow()
	if monitor == nil {
		return 1
	}

	widthMm, heightMm := monitor.GetPhysicalSize()
	if runtime.GOOS == "linux" && widthMm == 60 && heightMm == 60 { // Steam Deck incorrectly reports 6cm square!
		return 1
	}
	widthPx := monitor.GetVideoMode().Width

	return calculateDetectedScale(widthMm, widthPx)
}

func (w *window) moved(_ *glfw.Window, x, y int) {
	w.processMoved(x, y)
}
Repository: fyne-io/fyne
File: internal/driver/glfw/window_desktop.go (Go)

Code before:
			if videoMode := monitor.GetVideoMode(); x+videoMode.Width <= xOff || y+videoMode.Height <= yOff {
				continue
			}

			return monitor
		}
	}

	// try built-in function to detect monitor if above logic didn't succeed
	// if it doesn't work then return primary monitor as default
	monitor := w.viewport.GetMonitor()
	if monitor == nil {
		monitor = glfw.GetPrimaryMonitor()
	}
	return monitor
}

func (w *window) detectScale() float32 {
	// check if DPI detection is disabled
	env := os.Getenv(disableDPIDetectionEnvKey)
	switch env {
	case "True", "TRUE", "true", "T", "t", "1":
		return 1
	}

	if build.IsWayland { // Wayland controls scale through content scaling
		return 1
	}
	monitor := w.getMonitorForWindow()
	if monitor == nil {
		return 1
	}

	widthMm, heightMm := monitor.GetPhysicalSize()
	if runtime.GOOS == "linux" && widthMm == 60 && heightMm == 60 { // Steam Deck incorrectly reports 6cm square!
		return 1
	}
	widthPx := monitor.GetVideoMode().Width

	return calculateDetectedScale(widthMm, widthPx)
}

func (w *window) moved(_ *glfw.Window, x, y int) {
	w.processMoved(x, y)
}

func (w *window) resized(_ *glfw.Window, width, height int) {
	w.processResized(width, height)
}

func (w *window) scaled(_ *glfw.Window, x float32, y float32) {

Review comment (suggestion):
I think it would be wise to move the env check to after this check. In the case of Wayland, there is no need to check the environment variable at all and the compiler can just deadcode eliminate most of the function.

Code after:
			}
			if videoMode := monitor.GetVideoMode(); x+videoMode.Width <= xOff || y+videoMode.Height <= yOff {
				continue
			}

			return monitor
		}
	}

	// try built-in function to detect monitor if above logic didn't succeed
	// if it doesn't work then return primary monitor as default
	monitor := w.viewport.GetMonitor()
	if monitor == nil {
		monitor = glfw.GetPrimaryMonitor()
	}
	return monitor
}

func (w *window) detectScale() float32 {
	if build.IsWayland { // Wayland controls scale through content scaling
		return 1
	}

	// check if DPI detection is disabled
	env := os.Getenv(disableDPIDetectionEnvKey)
	if strings.EqualFold(env, "true") || strings.EqualFold(env, "t") || env == "1" {
		return 1
	}

	monitor := w.getMonitorForWindow()
	if monitor == nil {
		return 1
	}

	widthMm, heightMm := monitor.GetPhysicalSize()
	if runtime.GOOS == "linux" && widthMm == 60 && heightMm == 60 { // Steam Deck incorrectly reports 6cm square!
		return 1
	}
	widthPx := monitor.GetVideoMode().Width

	return calculateDetectedScale(widthMm, widthPx)
}

func (w *window) moved(_ *glfw.Window, x, y int) {
	w.processMoved(x, y)
}

func (w *window) resized(_ *glfw.Window, width, height int) {
	w.processResized(width, height)
}