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

Code before:
//     WritableRepository instance, then this method will fail with a
//     repository.ErrOperationNotSupported.
//
// Delete is backed by the repository system - this function calls
// into a scheme-specific implementation from a registered repository.
//
// Since: 2.0
func Delete(u fyne.URI) error {
	listable, err := CanList(u)
	if err != nil {
		return err
	}

	// Will delete if target is not a folder
	if !listable {
		return delete(u)
	}

	// Makes list of all contents of the folder
	list, err := List(u)
	if err != nil {
		return err
	}

	// Will delele if it is an empty folder
	if list == nil {
		return delete(u)
	}

	// Will delete all contents of target folder then delete the folder itself
	queue := list
	var folders []fyne.URI
	var files []fyne.URI

	// Lists paths of all sub-folders and files
	for len(queue) > 0 {
		currentPath := queue[0]
		queue = queue[1:]

		listable, _ := CanList(currentPath)

		if listable {
			list, err = List(currentPath)
			if err != nil {
				return err
			}
			folders = append(folders, currentPath)
			queue = append(queue, list...)
		} else {
			files = append(files, currentPath)
		}

Review comment (suggestion):
Per above comment, doing `if len(folderContents) == 0` is easier to follow than whatever a `nil` list might mean (if you don't know what the function that returns it does).

Code after:
	repo, err := repository.ForURI(u)
	if err != nil {
		return err
	}

	wrepo, ok := repo.(repository.WritableRepository)
	if !ok {
		return repository.ErrOperationNotSupported
	}

	return wrepo.Delete(u)
}

// DeleteAll destroys, deletes, or otherwise removes the resource referenced
// by the URI and any child resources for listable URIs.
//
// DeleteAll is backed by the repository system - this function calls
// into a scheme-specific implementation from a registered repository.
//
// Since: 2.7
func DeleteAll(u fyne.URI) error {
	repo, err := repository.ForURI(u)
	if err != nil {
		return err
	}

	drepo, ok := repo.(repository.DeleteAllRepository)
	if !ok {
		return repository.GenericDeleteAll(u)
	}

	return drepo.DeleteAll(u)
}

// Reader returns URIReadCloser set up to read from the resource that the
// URI references.
//
// This method can fail in several ways:
//
//   - Different permissions or credentials are required to read the
//     referenced resource.
//
//   - This URI scheme could represent some resources that can be read,
//     but this particular URI references a resources that is not
//     something that can be read.
//
//   - Attempting to set up the reader depended on a lower level
//     operation such as a network or filesystem access that has failed
//     in some way.
//
// Reader is backed by the repository system - this function calls
Repository: fyne-io/fyne
File: storage/uri.go (Go)

Code before:
// Since: 2.7
func DeleteAll(u fyne.URI) error {
	listable, err := CanList(u)
	if err != nil {
		return err
	}

	if !listable {
		return Delete(u)
	}

	children, err := List(u)
	if err != nil {
		return err
	}

	if len(children) == 0 {
		return Delete(u)
	}

	var folders []fyne.URI
	var files []fyne.URI
	for i := 0; i < len(children); i++ {
		listable, err = CanList(children[i])

		if err != nil {
			return err
		}

		if listable {
			grandChildren, err := List(children[i])
			if err != nil {
				return err
			}
			folders = append(folders, children[i])
			children = append(children, grandChildren...)
		} else {
			files = append(files, children[i])
		}
	}

	for i := len(files) - 1; i >= 0; i-- {
		err = Delete(files[i])
		if err != nil {
			return err
		}
	}

	for i := len(folders) - 1; i >= 0; i-- {
		err = Delete(folders[i])
		if err != nil {

Review comment (suggestion):
```suggestion
		listable, err = CanList(children[i])
		if err != nil {
```

Code after:
	}

	drepo, ok := repo.(repository.DeleteAllRepository)
	if !ok {
		return repository.GenericDeleteAll(u)
	}

	return drepo.DeleteAll(u)
}

// Reader returns URIReadCloser set up to read from the resource that the
// URI references.
//
// This method can fail in several ways:
//
//   - Different permissions or credentials are required to read the
//     referenced resource.
//
//   - This URI scheme could represent some resources that can be read,
//     but this particular URI references a resources that is not
//     something that can be read.
//
//   - Attempting to set up the reader depended on a lower level
//     operation such as a network or filesystem access that has failed
//     in some way.
//
// Reader is backed by the repository system - this function calls
// into a scheme-specific implementation from a registered repository.
//
// Since: 2.0
func Reader(u fyne.URI) (fyne.URIReadCloser, error) {
	repo, err := repository.ForURI(u)
	if err != nil {
		return nil, err
	}

	return repo.Reader(u)
}

// CanRead determines if a given URI could be written to using the Reader()
// method. It is preferred to check if a URI is readable using this method
// before calling Reader(), because the underlying operations required to
// attempt to read and then report an error may be slower than the operations
// needed to test if a URI is readable. Keep in mind however that even if
// CanRead returns true, you must still do appropriate error handling for
// Reader(), as the underlying filesystem may have changed since you called
// CanRead.
//
// The non-existence of a resource should not be treated as an error. In other
// words, a Repository implementation which for some URI u returns false, nil
// for Exists(u), CanRead(u) should also return false, nil.
Repository: fyne-io/fyne
File: internal/driver/glfw/loop.go (Go)

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

type funcData struct {
	f    func()
	done chan struct{} // Zero allocation signalling channel
}

// channel for queuing functions on the main thread
var funcQueue = async.NewUnboundedChan[funcData]()
var running, drained atomic.Bool

// Arrange that main.main runs on main thread.
func init() {
	runtime.LockOSThread()
	async.SetMainGoroutine()
}

// force a function f to run on the main thread
func runOnMain(f func()) {
	runOnMainWithWait(f, true)
}

// force a function f to run on the main thread and specify if we should wait for it to return
func runOnMainWithWait(f func(), wait bool) {
	// If we are on main just execute - otherwise add it to the main queue and wait.
	// The "running" variable is normally false when we are on the main thread.
	if (!running.Load() && async.IsMainGoroutine()) || drained.Load() {
		f()
		return
	}

	if wait {
		done := common.DonePool.Get()
		defer common.DonePool.Put(done)

		funcQueue.In() <- funcData{f: f, done: done}
		<-done
	} else {
		funcQueue.In() <- funcData{f: f}
	}
}

func (d *gLDriver) drawSingleFrame() {
	refreshed := false
	for _, win := range d.windowList() {
		w := win.(*window)
		if w.closing {
			continue
		}


Review comment (suggestion):
I think this comment should be updated, something like

// If the app main loop is not yet running, run f directly only if we are on the main goroutine. Otherwise, queue it as normal to run after the app starts.

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

type funcData struct {
	f    func()
	done chan struct{} // Zero allocation signalling channel
}

// channel for queuing functions on the main thread
var funcQueue = async.NewUnboundedChan[funcData]()
var running, drained atomic.Bool

// Arrange that main.main runs on main thread.
func init() {
	runtime.LockOSThread()
	async.SetMainGoroutine()
}

// force a function f to run on the main thread
func runOnMain(f func()) {
	runOnMainWithWait(f, true)
}

// force a function f to run on the main thread and specify if we should wait for it to return
func runOnMainWithWait(f func(), wait bool) {
	// If we are on main before app run just execute - otherwise add it to the main queue and wait.
	// We also need to run it as-is if the app is in the process of shutting down as the queue will be stopped.
	if (!running.Load() && async.IsMainGoroutine()) || drained.Load() {
		f()
		return
	}

	if wait {
		done := common.DonePool.Get()
		defer common.DonePool.Put(done)

		funcQueue.In() <- funcData{f: f, done: done}
		<-done
	} else {
		funcQueue.In() <- funcData{f: f}
	}
}

func (d *gLDriver) drawSingleFrame() {
	refreshed := false
	for _, win := range d.windowList() {
		w := win.(*window)
		if w.closing {
			continue
		}
Repository: fyne-io/fyne
File: internal/driver/glfw/loop.go (Go)

Code before:
			c, ok := w.Canvas().(*glCanvas)
			if !ok {
				return
			}
			c.applyThemeOutOfTreeObjects()
			c.reloadScale()
		})
	})

	if f := fyne.CurrentApp().Lifecycle().(*app.Lifecycle).OnStarted(); f != nil {
		f()
	}

	eventTick := time.NewTicker(time.Second / 60)
	for {
		select {
		case <-d.done:
			eventTick.Stop()
			d.Terminate()
			l := fyne.CurrentApp().Lifecycle().(*app.Lifecycle)
			if f := l.OnStopped(); f != nil {
				l.QueueEvent(f)
			}

			for len(funcQueue.Out()) > 0 {
				f := <-funcQueue.Out()
				if f.done != nil {
					f.done <- struct{}{}
				}
			}
			drained.Store(true)
			funcQueue.Close()
			return
		case f := <-funcQueue.Out():
			f.f()
			if f.done != nil {
				f.done <- struct{}{}
			}
		case <-eventTick.C:
			d.pollEvents()
			for i := 0; i < len(d.windows); i++ {
				w := d.windows[i].(*window)
				if w.viewport == nil {
					continue
				}

				if w.viewport.ShouldClose() {
					d.destroyWindow(w, i)
					i-- // Trailing windows are moved forward one step.
					continue
				}

Review comment (suggestion):
A comment here could be nice too to explain the goal of this code block (I assume it's so that goroutines blocked on a `fyne.DoAndWait` will not be blocked forever)

Code after:
			c, ok := w.Canvas().(*glCanvas)
			if !ok {
				return
			}
			c.applyThemeOutOfTreeObjects()
			c.reloadScale()
		})
	})

	if f := fyne.CurrentApp().Lifecycle().(*app.Lifecycle).OnStarted(); f != nil {
		f()
	}

	eventTick := time.NewTicker(time.Second / 60)
	for {
		select {
		case <-d.done:
			eventTick.Stop()
			d.Terminate()
			l := fyne.CurrentApp().Lifecycle().(*app.Lifecycle)
			if f := l.OnStopped(); f != nil {
				l.QueueEvent(f)
			}

			// as we are shutting down make sure we drain the pending funcQueue and close it out.
			for len(funcQueue.Out()) > 0 {
				f := <-funcQueue.Out()
				if f.done != nil {
					f.done <- struct{}{}
				}
			}
			drained.Store(true)
			funcQueue.Close()
			return
		case f := <-funcQueue.Out():
			f.f()
			if f.done != nil {
				f.done <- struct{}{}
			}
		case <-eventTick.C:
			d.pollEvents()
			for i := 0; i < len(d.windows); i++ {
				w := d.windows[i].(*window)
				if w.viewport == nil {
					continue
				}

				if w.viewport.ShouldClose() {
					d.destroyWindow(w, i)
					i-- // Trailing windows are moved forward one step.
					continue
Repository: fyne-io/fyne
File: widget/entry.go (Go)

Code before:
// Implements: desktop.Mouseable
func (e *Entry) MouseUp(m *desktop.MouseEvent) {
	e.syncSelectable()
	start, _ := e.sel.selection()
	if start == -1 && e.sel.selecting && !e.selectKeyDown {
		e.sel.selecting = false
	}
}

// Redo un-does the last undo action.
//
// Since: 2.5
func (e *Entry) Redo() {
	newText, action := e.undoStack.Redo(e.Text)
	modify, ok := action.(*entryModifyAction)
	if !ok {
		return
	}
	pos := modify.Position
	if !modify.Delete {
		pos += len(modify.Text)
	}
	e.updateText(newText, false)
	e.CursorRow, e.CursorColumn = e.rowColFromTextPos(pos)
	e.syncSelectable()
	cb := e.OnChanged
	if cb != nil {
		cb(newText) // We know that the text has changed.
	}
	e.Refresh()
}

func (e *Entry) Refresh() {
	e.minCache = fyne.Size{}

	if e.sel != nil {
		e.sel.style = e.TextStyle
		e.sel.theme = e.Theme()
		e.sel.focussed = e.focused
		e.sel.Refresh()
	}
	e.BaseWidget.Refresh()
}

// SelectedText returns the text currently selected in this Entry.
// If there is no selection it will return the empty string.
func (e *Entry) SelectedText() string {
	return e.sel.SelectedText()
}

// SetMinRowsVisible forces a multi-line entry to show `count` number of rows without scrolling.

Review comment (suggestion):
This could be simplified:

```go
	if cb := e.OnChanged; cb != nil {
```

Code after:
// Implements: desktop.Mouseable
func (e *Entry) MouseUp(m *desktop.MouseEvent) {
	e.syncSelectable()
	start, _ := e.sel.selection()
	if start == -1 && e.sel.selecting && !e.selectKeyDown {
		e.sel.selecting = false
	}
}

// Redo un-does the last undo action.
//
// Since: 2.5
func (e *Entry) Redo() {
	newText, action := e.undoStack.Redo(e.Text)
	modify, ok := action.(*entryModifyAction)
	if !ok {
		return
	}
	pos := modify.Position
	if !modify.Delete {
		pos += len(modify.Text)
	}
	e.updateText(newText, false)
	e.CursorRow, e.CursorColumn = e.rowColFromTextPos(pos)
	e.syncSelectable()
	if e.OnChanged != nil {
		e.OnChanged(newText)
	}
	e.Refresh()
}

func (e *Entry) Refresh() {
	e.minCache = fyne.Size{}

	if e.sel != nil {
		e.sel.style = e.TextStyle
		e.sel.theme = e.Theme()
		e.sel.focussed = e.focused
		e.sel.Refresh()
	}
	e.BaseWidget.Refresh()
}

// SelectedText returns the text currently selected in this Entry.
// If there is no selection it will return the empty string.
func (e *Entry) SelectedText() string {
	return e.sel.SelectedText()
}

// SetMinRowsVisible forces a multi-line entry to show `count` number of rows without scrolling.
// This is not a validation or requirement, it just impacts the minimum visible size.
Repository: fyne-io/fyne
File: widget/textgrid.go (Go)

Code before:
	cols int

	objects []fyne.CanvasObject
}

func (t *textGridRowRenderer) appendTextCell(str rune) {
	th := t.obj.text.text.Theme()
	v := fyne.CurrentApp().Settings().ThemeVariant()

	text := canvas.NewText(string(str), th.Color(theme.ColorNameForeground, v))
	text.TextStyle.Monospace = true

	bg := canvas.NewRectangle(color.Transparent)

	ul := canvas.NewLine(color.Transparent)

	t.objects = append(t.objects, bg, text, ul)
}

func (t *textGridRowRenderer) refreshCell(col int) {
	pos := t.cols + col
	if pos*3+1 >= len(t.objects) {
		return
	}

	if len(t.obj.text.text.Rows[t.obj.row].Cells) > col {
		cell := t.obj.text.text.Rows[t.obj.row].Cells[col]
		t.setCellRune(cell.Rune, pos, cell.Style, t.obj.text.text.Rows[t.obj.row].Style)
	}
}

func (t *textGridRowRenderer) setCellRune(str rune, pos int, style, rowStyle TextGridStyle) {
	if str == 0 {
		str = ' '
	}
	rect := t.objects[pos*3].(*canvas.Rectangle)
	text := t.objects[pos*3+1].(*canvas.Text)
	underline := t.objects[pos*3+2].(*canvas.Line)

	th := t.obj.text.text.Theme()
	v := fyne.CurrentApp().Settings().ThemeVariant()
	fg := th.Color(theme.ColorNameForeground, v)
	text.TextSize = th.Size(theme.SizeNameText)

	var underlineStrokeWidth float32 = 1
	var underlineStrokeColor color.Color = color.Transparent
	textStyle := fyne.TextStyle{}
	if style != nil {
		textStyle = style.Style()
	} else if rowStyle != nil {
		textStyle = rowStyle.Style()

Review comment (suggestion):
Minor style suggestion - I think readability can benefit from assigning `row := t.obj.text.text.Rows[t.obj.row]` to avoid the repeats of this slightly cumbersome access chain

Code after:
	cols int

	objects []fyne.CanvasObject
}

func (t *textGridRowRenderer) appendTextCell(str rune) {
	th := t.obj.text.text.Theme()
	v := fyne.CurrentApp().Settings().ThemeVariant()

	text := canvas.NewText(string(str), th.Color(theme.ColorNameForeground, v))
	text.TextStyle.Monospace = true

	bg := canvas.NewRectangle(color.Transparent)

	ul := canvas.NewLine(color.Transparent)

	t.objects = append(t.objects, bg, text, ul)
}

func (t *textGridRowRenderer) refreshCell(col int) {
	pos := t.cols + col
	if pos*3+1 >= len(t.objects) {
		return
	}

	row := t.obj.text.text.Rows[t.obj.row]

	if len(row.Cells) > col {
		cell := row.Cells[col]
		t.setCellRune(cell.Rune, pos, cell.Style, row.Style)
	}
}

func (t *textGridRowRenderer) setCellRune(str rune, pos int, style, rowStyle TextGridStyle) {
	if str == 0 {
		str = ' '
	}
	rect := t.objects[pos*3].(*canvas.Rectangle)
	text := t.objects[pos*3+1].(*canvas.Text)
	underline := t.objects[pos*3+2].(*canvas.Line)

	th := t.obj.text.text.Theme()
	v := fyne.CurrentApp().Settings().ThemeVariant()
	fg := th.Color(theme.ColorNameForeground, v)
	text.TextSize = th.Size(theme.SizeNameText)

	var underlineStrokeWidth float32 = 1
	var underlineStrokeColor color.Color = color.Transparent
	textStyle := fyne.TextStyle{}
	if style != nil {
		textStyle = style.Style()
Repository: fyne-io/fyne
File: internal/widget/base.go (Go)

Code before:
		return
	}

	w.size = size

	impl := w.super()
	if impl == nil {
		return
	}
	cache.Renderer(impl).Layout(size)
}

// Position gets the current position of this widget, relative to its parent.
func (w *Base) Position() fyne.Position {
	return w.position
}

// Move the widget to a new position, relative to its parent.
// Note this should not be used if the widget is being managed by a Layout within a Container.
func (w *Base) Move(pos fyne.Position) {
	if w.Position() == pos {
		return
	}
	w.position = pos

	Repaint(w.super())
}

// MinSize for the widget - it should never be resized below this value.
func (w *Base) MinSize() fyne.Size {
	impl := w.super()

	r := cache.Renderer(impl)
	if r == nil {
		return fyne.NewSize(0, 0)
	}

	return r.MinSize()
}

// Visible returns whether or not this widget should be visible.
// Note that this may not mean it is currently visible if a parent has been hidden.
func (w *Base) Visible() bool {
	return !w.hidden
}

// Show this widget so it becomes visible
func (w *Base) Show() {
	if !w.hidden {
		return // Visible already
	}

Review comment (suggestion):
This is a bit nitpicky but would you mind updating the two base widgets to have the lines match in terms of empty lines? I.e. move the line setting the position down one line and keep the empty line between the if statement instead.

Code after:
		return
	}

	w.size = size

	impl := w.super()
	if impl == nil {
		return
	}
	cache.Renderer(impl).Layout(size)
}

// Position gets the current position of this widget, relative to its parent.
func (w *Base) Position() fyne.Position {
	return w.position
}

// Move the widget to a new position, relative to its parent.
// Note this should not be used if the widget is being managed by a Layout within a Container.
func (w *Base) Move(pos fyne.Position) {
	if w.Position() == pos {
		return
	}

	w.position = pos

	Repaint(w.super())
}

// MinSize for the widget - it should never be resized below this value.
func (w *Base) MinSize() fyne.Size {
	impl := w.super()

	r := cache.Renderer(impl)
	if r == nil {
		return fyne.NewSize(0, 0)
	}

	return r.MinSize()
}

// Visible returns whether or not this widget should be visible.
// Note that this may not mean it is currently visible if a parent has been hidden.
func (w *Base) Visible() bool {
	return !w.hidden
}

// Show this widget so it becomes visible
func (w *Base) Show() {
	if !w.hidden {
		return // Visible already
Repository: fyne-io/fyne
File: canvas/rectangle.go (Go)

Code before:
	StrokeColor color.Color // The rectangle stroke color
	StrokeWidth float32     // The stroke width of the rectangle
	// The radius of the rectangle corners
	//
	// Since: 2.4
	CornerRadius float32

	// Enforce an aspect ratio for the rectangle, the content will be made shorter or narrower
	// to meet the requested aspect, if set.
	//
	// Since: 2.7
	Aspect float32

	// The `CornerRadius` field sets the default radius for all corners. If a specific corner
	// radius (such as `TopRightCornerRadius`, `TopLeftCornerRadius`, `BottomRightCornerRadius`,
	// or `BottomLeftCornerRadius`) is set to a value greater than `CornerRadius`, that value
	// will override `CornerRadius` for the respective corner. Otherwise, `CornerRadius` is used.
	//
	// Since: 2.7
	TopRightCornerRadius    float32
	TopLeftCornerRadius     float32
	BottomRightCornerRadius float32
	BottomLeftCornerRadius  float32
}

// Hide will set this rectangle to not be visible
func (r *Rectangle) Hide() {
	r.baseObject.Hide()

	repaint(r)
}

// Move the rectangle to a new position, relative to its parent / canvas
func (r *Rectangle) Move(pos fyne.Position) {
	if r.Position() == pos {
		return
	}

	r.baseObject.Move(pos)

	repaint(r)
}

// Refresh causes this rectangle to be redrawn with its configured state.
func (r *Rectangle) Refresh() {
	Refresh(r)
}

// Resize on a rectangle updates the new size of this object.
// If it has a stroke width this will cause it to Refresh.
// If Aspect is non-zero it may cause the rectangle to be smaller than the requested size.

Review comment (suggestion):
Unfortunately (I think...) This document only applies to the first item in the list of fields.
The note about version needs to be on both and the documentation re-jigged to describe the field rather than the concept.

Wider documentation could be added to the rectangle page on docs.fyne.io.

Code after:
	StrokeColor color.Color // The rectangle stroke color
	StrokeWidth float32     // The stroke width of the rectangle
	// The radius of the rectangle corners
	//
	// Since: 2.4
	CornerRadius float32

	// Enforce an aspect ratio for the rectangle, the content will be made shorter or narrower
	// to meet the requested aspect, if set.
	//
	// Since: 2.7
	Aspect float32

	// The radius of the rectangle top-right corner only.
	//
	// Since: 2.7
	TopRightCornerRadius float32

	// The radius of the rectangle top-left corner only.
	//
	// Since: 2.7
	TopLeftCornerRadius float32

	// The radius of the rectangle bottom-right corner only.
	//
	// Since: 2.7
	BottomRightCornerRadius float32

	// The radius of the rectangle bottom-left corner only.
	//
	// Since: 2.7
	BottomLeftCornerRadius float32
}

// Hide will set this rectangle to not be visible
func (r *Rectangle) Hide() {
	r.baseObject.Hide()

	repaint(r)
}

// Move the rectangle to a new position, relative to its parent / canvas
func (r *Rectangle) Move(pos fyne.Position) {
	if r.Position() == pos {
		return
	}

	r.baseObject.Move(pos)

	repaint(r)
}