ronantakizawa/codereview-bench huggingface dataset:
Repository: zyedidia/micro
File: internal/buffer/settings.go (Go)

Code before:
)

func (b *Buffer) ReloadSettings(reloadFiletype bool) {
	oldFiletype := b.Settings["filetype"]
	settings := config.ParsedSettings()
	config.UpdatePathGlobLocals(settings, b.AbsPath)

	_, local := b.LocalSettings["filetype"]
	_, volatile := config.VolatileSettings["filetype"]
	if reloadFiletype && !local && !volatile {
		// need to update filetype before updating other settings based on it
		b.Settings["filetype"] = "unknown"
		if v, ok := settings["filetype"]; ok {
			b.Settings["filetype"] = v
		}
	}

	// update syntax rules, which will also update filetype if needed
	b.UpdateRules()

	curFiletype := b.Settings["filetype"]
	if oldFiletype != curFiletype {
		b.doCallbacks("filetype", oldFiletype, curFiletype)
	}

	config.UpdateFileTypeLocals(settings, curFiletype.(string))

	for k, v := range config.DefaultCommonSettings() {
		if k == "filetype" {
			// prevent recursion
			continue
		}
		if _, ok := config.VolatileSettings[k]; ok {
			// reload should not override volatile settings
			continue
		}
		if _, ok := b.LocalSettings[k]; ok {
			// reload should not override local settings
			continue
		}
		if _, ok := settings[k]; ok {
			b.DoSetOptionNative(k, settings[k])
		} else {
			b.DoSetOptionNative(k, v)
		}
	}
}

func (b *Buffer) DoSetOptionNative(option string, nativeValue interface{}) {
	oldValue := b.Settings[option]
	if reflect.DeepEqual(oldValue, nativeValue) {

Review comment (suggestion):
We could also set `curFiletype` and `oldFiletype` to`b.Settings["filetype"].(string)` instead of type-asserting it here.

Code after:
)

func (b *Buffer) ReloadSettings(reloadFiletype bool) {
	settings := config.ParsedSettings()
	config.UpdatePathGlobLocals(settings, b.AbsPath)

	oldFiletype := b.Settings["filetype"].(string)

	_, local := b.LocalSettings["filetype"]
	_, volatile := config.VolatileSettings["filetype"]
	if reloadFiletype && !local && !volatile {
		// need to update filetype before updating other settings based on it
		b.Settings["filetype"] = "unknown"
		if v, ok := settings["filetype"]; ok {
			b.Settings["filetype"] = v
		}
	}

	// update syntax rules, which will also update filetype if needed
	b.UpdateRules()

	curFiletype := b.Settings["filetype"].(string)
	if oldFiletype != curFiletype {
		b.doCallbacks("filetype", oldFiletype, curFiletype)
	}

	config.UpdateFileTypeLocals(settings, curFiletype)

	for k, v := range config.DefaultCommonSettings() {
		if k == "filetype" {
			// prevent recursion
			continue
		}
		if _, ok := config.VolatileSettings[k]; ok {
			// reload should not override volatile settings
			continue
		}
		if _, ok := b.LocalSettings[k]; ok {
			// reload should not override local settings
			continue
		}
		if _, ok := settings[k]; ok {
			b.DoSetOptionNative(k, settings[k])
		} else {
			b.DoSetOptionNative(k, v)
		}
	}
}

func (b *Buffer) DoSetOptionNative(option string, nativeValue interface{}) {
	oldValue := b.Settings[option]
Repository: zyedidia/micro
File: internal/views/splits.go (Go)

Code before:
// else accordingly
func (n *Node) Unsplit() bool {
	if !n.IsLeaf() || n.parent == nil {
		return false
	}
	ind := 0
	for i, c := range n.parent.children {
		if c.id == n.id {
			ind = i
		}
	}
	if n.parent.Kind == STVert {
		n.parent.unsplit(ind, true)
	} else {
		n.parent.unsplit(ind, false)
	}

	if n.parent.IsLeaf() {
		return n.parent.Unsplit()
	}

	n.parent.simplify()
	return true
}

// Simplify removes unnecessary chained parents
func (n *Node) simplify() {
	if n.parent == nil || len(n.children) != 1 {
		return
	}

	ind := 0
	for i, c := range n.parent.children {
		if c.id == n.id {
			ind = i
		}
	}

	parent := n.parent
	kind := n.Kind
	parent.children[ind] = n.children[0]
	parent.children[ind].parent = parent
	parent.children[ind].Kind = kind
	parent.simplify()
}

// String returns the string form of the node and all children (used for debugging)
func (n *Node) String() string {
	var strf func(n *Node, ident int) string
	strf = func(n *Node, ident int) string {
		marker := "|"

Review comment (refactor):
nit: Maybe clarify a bit better what this actually means, e.g. "Simplify removes unnecessary intermediate parents that have just one child"?

(Since just from looking at the code it is not immediately obvious that the check `len(n.children) != 1` is crucial.)

Code after:
// Unsplit deletes this split and resizes everything
// else accordingly
func (n *Node) Unsplit() bool {
	if !n.IsLeaf() || n.parent == nil {
		return false
	}
	ind := 0
	for i, c := range n.parent.children {
		if c.id == n.id {
			ind = i
		}
	}
	n.parent.unsplit(ind)
	if n.parent.IsLeaf() {
		return n.parent.Unsplit()
	}

	n.parent.flatten()
	return true
}

// flattens the tree by removing unnecessary intermediate parents that have only one child
// and handles the side effect of it
func (n *Node) flatten() {
	if n.parent == nil || len(n.children) != 1 {
		return
	}

	ind := 0
	for i, c := range n.parent.children {
		if c.id == n.id {
			ind = i
		}
	}

	// Replace current node with its child node to remove chained parent
	successor := n.children[0]
	n.parent.children[ind] = successor
	successor.parent = n.parent

	// Maintain the tree in a consistent state: any child node's kind (horiz vs vert)
	// should be the opposite of its parent's kind.
	if successor.IsLeaf() {
		successor.Kind = n.Kind
	} else {
		// If the successor node has children, that means it is a chained parent as well.
		// Therefore it can be replaced by its own children.
		origsize := len(n.parent.children)

		// Let's say we have 5 children and want to replace [2] with its children [a] [b] [c]
		// [0] [1] [2] [3] [4] --> [0] [1] [a] [b] [c] [3] [4]
Repository: zyedidia/micro
File: internal/views/splits.go (Go)

Code before:

// Unsplit deletes this split and resizes everything
// else accordingly
func (n *Node) Unsplit() bool {
	if !n.IsLeaf() || n.parent == nil {
		return false
	}
	ind := 0
	for i, c := range n.parent.children {
		if c.id == n.id {
			ind = i
		}
	}
	n.parent.unsplit(ind)
	if n.parent.IsLeaf() {
		return n.parent.Unsplit()
	}

	n.parent.simplify()
	return true
}

// Simplify removes unnecessary chained parents
func (n *Node) simplify() {
	if n.parent == nil || len(n.children) != 1 {
		n.markResize()
		return
	}

	ind := 0
	for i, c := range n.parent.children {
		if c.id == n.id {
			ind = i
		}
	}

	parent := n.parent
	kind := n.Kind
	parent.children[ind] = n.children[0]
	parent.children[ind].parent = parent
	parent.children[ind].Kind = kind
	parent.simplify()
}

// String returns the string form of the node and all children (used for debugging)
func (n *Node) String() string {
	var strf func(n *Node, ident int) string
	strf = func(n *Node, ident int) string {
		marker := "|"
		if n.Kind == STHoriz {
			marker = "-"

Review comment (bug):
This fixes a regression in https://github.com/zyedidia/micro/pull/3708/commits/ec4ca05aa21ecc2badc3dc83afa6a792781c5cd1 so this commit should be squashed into https://github.com/zyedidia/micro/pull/3708/commits/ec4ca05aa21ecc2badc3dc83afa6a792781c5cd1 ?

Also just a question: could you describe an example test when the lack of `markResize()` here is causing a problem? (In my tests, proportional scaling actually seems broken and not working most of the time, either with or without this PR, either with or without `markResize()` here.)

Code after:

// Unsplit deletes this split and resizes everything
// else accordingly
func (n *Node) Unsplit() bool {
	if !n.IsLeaf() || n.parent == nil {
		return false
	}
	ind := 0
	for i, c := range n.parent.children {
		if c.id == n.id {
			ind = i
		}
	}
	n.parent.unsplit(ind)
	if n.parent.IsLeaf() {
		return n.parent.Unsplit()
	}

	n.parent.flatten()
	return true
}

// flattens the tree by removing unnecessary intermediate parents that have only one child
// and handles the side effect of it
func (n *Node) flatten() {
	if n.parent == nil || len(n.children) != 1 {
		return
	}

	ind := 0
	for i, c := range n.parent.children {
		if c.id == n.id {
			ind = i
		}
	}

	// Replace current node with its child node to remove chained parent
	successor := n.children[0]
	n.parent.children[ind] = successor
	successor.parent = n.parent

	// Maintain the tree in a consistent state: any child node's kind (horiz vs vert)
	// should be the opposite of its parent's kind.
	if successor.IsLeaf() {
		successor.Kind = n.Kind
	} else {
		// If the successor node has children, that means it is a chained parent as well.
		// Therefore it can be replaced by its own children.
		origsize := len(n.parent.children)

		// Let's say we have 5 children and want to replace [2] with its children [a] [b] [c]
Repository: zyedidia/micro
File: internal/action/actions.go (Go)

Code before:
	return true
}

// ClearInfo clears the infobar
func (h *BufPane) ClearInfo() bool {
	if InfoBar.Msg == "" {
		return false
	}
	InfoBar.Message("")
	return true
}

// ForceQuit closes the current tab or view even if there are unsaved changes
// (no prompt)
func (h *BufPane) ForceQuit() bool {
	h.Buf.Close()
	if len(MainTab().Panes) > 1 {
		h.Unsplit()
	} else if len(Tabs.List) > 1 {
		Tabs.RemoveTab(h.splitID)
	} else {
		screen.Screen.Fini()
		InfoBar.Close()
		runtime.Goexit()
	}
	return true
}

// savePrompt displays a prompt to save the buffer before proceeding with a
// different action or command
func (h *BufPane) savePrompt(action string, callback func()) {
	InfoBar.YNPrompt("Save changes to "+h.Buf.GetName()+" before closing? (y,n,esc)", func(yes, canceled bool) {
		if !canceled && !yes {
			callback()
		} else if !canceled && yes {
			h.SaveCB(action, callback)
		}
	})
}

// Quit this will close the current tab or view that is open
func (h *BufPane) Quit() bool {
	if h.Buf.Modified() {
		if h.Buf.Shared() {
			h.ForceQuit()
			return true
		}

		if config.GlobalSettings["autosave"].(float64) > 0 && h.Buf.Path != "" {
			// autosave on means we automatically save when quitting
			h.SaveCB("Quit", func() {

Review comment (refactor):
> This slightly changes the open and term command to be similar with the
Quit action, where the buffer or pane is replaced after the prompts are
completed if "n" wasn't pressed after the 1st prompt.

This sounds like there is some functional change, not just code refactoring, but AFAICS there is no functional change. What am I missing?

Code after:
	return true
}

// ClearInfo clears the infobar
func (h *BufPane) ClearInfo() bool {
	if InfoBar.Msg == "" {
		return false
	}
	InfoBar.Message("")
	return true
}

// ForceQuit closes the current tab or view even if there are unsaved changes
// (no prompt)
func (h *BufPane) ForceQuit() bool {
	h.Buf.Close()
	if len(MainTab().Panes) > 1 {
		h.Unsplit()
	} else if len(Tabs.List) > 1 {
		Tabs.RemoveTab(h.splitID)
	} else {
		screen.Screen.Fini()
		InfoBar.Close()
		runtime.Goexit()
	}
	return true
}

// closePrompt displays a prompt to save the buffer before closing it to proceed
// with a different action or command
func (h *BufPane) closePrompt(action string, callback func()) {
	InfoBar.YNPrompt("Save changes to "+h.Buf.GetName()+" before closing? (y,n,esc)", func(yes, canceled bool) {
		if !canceled && !yes {
			callback()
		} else if !canceled && yes {
			h.SaveCB(action, callback)
		}
	})
}

// Quit this will close the current tab or view that is open
func (h *BufPane) Quit() bool {
	if h.Buf.Modified() && !h.Buf.Shared() {
		if config.GlobalSettings["autosave"].(float64) > 0 && h.Buf.Path != "" {
			// autosave on means we automatically save when quitting
			h.SaveCB("Quit", func() {
				h.ForceQuit()
			})
		} else {
			h.closePrompt("Quit", func() {
				h.ForceQuit()
Repository: zyedidia/micro
File: internal/action/command.go (Go)

Code before:

// OpenCmd opens a new buffer with a given filename
func (h *BufPane) OpenCmd(args []string) {
	if len(args) > 0 {
		filename := args[0]
		// the filename might or might not be quoted, so unquote first then join the strings.
		args, err := shellquote.Split(filename)
		if err != nil {
			InfoBar.Error("Error parsing args ", err)
			return
		}
		if len(args) == 0 {
			return
		}
		filename = strings.Join(args, " ")

		open := func() {
			b, err := buffer.NewBufferFromFile(filename, buffer.BTDefault)
			if err != nil {
				InfoBar.Error(err)
				return
			}
			h.OpenBuffer(b)
		}
		if h.Buf.Modified() {
			InfoBar.YNPrompt("Save changes to "+h.Buf.GetName()+" before closing? (y,n,esc)", func(yes, canceled bool) {
				if !canceled && !yes {
					open()
				} else if !canceled && yes {
					h.Save()
					open()
				}
			})
		} else {
			open()
		}
	} else {
		InfoBar.Error("No filename")
	}
}

// ToggleLogCmd toggles the log view
func (h *BufPane) ToggleLogCmd(args []string) {
	if h.Buf.Type != buffer.BTLog {
		h.OpenLogBuf()
	} else {
		h.Quit()
	}
}

// ReloadCmd reloads all files (syntax files, colorschemes, plugins...)

Review comment (suggestion):
Passing action="Save" will cause executing `onSave` callback?

Reminds me of https://github.com/zyedidia/micro/pull/3689#discussion_r1990022986

At the moment I'm inclined to think it's ok.

Code after:

// OpenCmd opens a new buffer with a given filename
func (h *BufPane) OpenCmd(args []string) {
	if len(args) > 0 {
		filename := args[0]
		// the filename might or might not be quoted, so unquote first then join the strings.
		args, err := shellquote.Split(filename)
		if err != nil {
			InfoBar.Error("Error parsing args ", err)
			return
		}
		if len(args) == 0 {
			return
		}
		filename = strings.Join(args, " ")

		open := func() {
			b, err := buffer.NewBufferFromFile(filename, buffer.BTDefault)
			if err != nil {
				InfoBar.Error(err)
				return
			}
			h.OpenBuffer(b)
		}
		if h.Buf.Modified() && !h.Buf.Shared() {
			h.closePrompt("Save", open)
		} else {
			open()
		}
	} else {
		InfoBar.Error("No filename")
	}
}

// ToggleLogCmd toggles the log view
func (h *BufPane) ToggleLogCmd(args []string) {
	if h.Buf.Type != buffer.BTLog {
		h.OpenLogBuf()
	} else {
		h.Quit()
	}
}

// ReloadCmd reloads all files (syntax files, colorschemes, plugins...)
func (h *BufPane) ReloadCmd(args []string) {
	reloadRuntime(true)
}

// ReloadConfig reloads only the configuration
func ReloadConfig() {
	reloadRuntime(false)
Repository: zyedidia/micro
File: internal/config/settings.go (Go)

Code before:
							continue
						}
					}
				}
			}
			continue
		}

		if k == "autosave" {
			// if autosave is a boolean convert it to float
			s, ok := v.(bool)
			if ok {
				if s {
					parsedSettings["autosave"] = 8.0
				} else {
					parsedSettings["autosave"] = 0.0
				}
			}
			continue
		}

		if k == "indentchar" {
			// migrate to "indenttabchar" from "indentchar"
			parsedSettings["indenttabchar"] = v
			err = errors.New("indentchar has been deprecated, use indenttabchar instead")
		}

		if _, ok := defaults[k]; ok {
			if e := verifySetting(k, v, defaults[k]); e != nil {
				err = e
				parsedSettings[k] = defaults[k]
				continue
			}
		}
	}
	return err
}

func ReadSettings() error {
	parsedSettings = make(map[string]interface{})
	filename := filepath.Join(ConfigDir, "settings.json")
	if _, e := os.Stat(filename); e == nil {
		input, err := os.ReadFile(filename)
		if err != nil {
			settingsParseError = true
			return errors.New("Error reading settings.json file: " + err.Error())
		}
		if !strings.HasPrefix(string(input), "null") {
			// Unmarshal the input into the parsed map
			err = json5.Unmarshal(input, &parsedSettings)
			if err != nil {

Review comment (suggestion):
> I am happy to revert the prompt message of telling user `indentchar` is deprecated, if people/maintainer finds it controversial.

Definitely!
You already added the mechanism to convert it, which is fine. We can add a deprecation note here.
Most probably we should only overwrite `indenttabchar` in case it doesn't exist yet. Since `indenttabchar` can be parsed later than `indentchar` it should be handled outside the loop (just store it here).

Code after:
						}
					}
				}
			}
			continue
		}

		if k == "autosave" {
			// if autosave is a boolean convert it to float
			s, ok := v.(bool)
			if ok {
				if s {
					parsedSettings["autosave"] = 8.0
				} else {
					parsedSettings["autosave"] = 0.0
				}
			}
			continue
		}

		if _, ok := defaults[k]; ok {
			if e := verifySetting(k, v, defaults[k]); e != nil {
				err = e
				parsedSettings[k] = defaults[k]
				continue
			}
		}
	}
	return err
}

func ReadSettings() error {
	parsedSettings = make(map[string]interface{})
	filename := filepath.Join(ConfigDir, "settings.json")
	if _, e := os.Stat(filename); e == nil {
		input, err := os.ReadFile(filename)
		if err != nil {
			settingsParseError = true
			return errors.New("Error reading settings.json file: " + err.Error())
		}
		if !strings.HasPrefix(string(input), "null") {
			// Unmarshal the input into the parsed map
			err = json5.Unmarshal(input, &parsedSettings)
			if err != nil {
				settingsParseError = true
				return errors.New("Error reading settings.json: " + err.Error())
			}
			err = validateParsedSettings()
			if err != nil {
				return err
			}
Repository: zyedidia/micro
File: internal/display/bufwindow.go (Go)

Code before:
	tabsize := util.IntOpt(b.Settings["tabsize"])
	colorcolumn := util.IntOpt(b.Settings["colorcolumn"])

	// this represents the current draw position
	// within the current window
	vloc := buffer.Loc{X: 0, Y: 0}
	if softwrap {
		// the start line may be partially out of the current window
		vloc.Y = -w.StartLine.Row
	}

	// this represents the current draw position in the buffer (char positions)
	bloc := buffer.Loc{X: -1, Y: w.StartLine.Line}

	cursors := b.GetCursors()

	curStyle := config.DefStyle

	// Parse showchars which is in the format of option1=val1,option2=val2,...
	spacechars := " "
	tabchars := b.Settings["indentchar"].(string)
	indentspacechars := " "
	indenttabchars := b.Settings["indentchar"].(string)

	charsentries := strings.Split(b.Settings["showchars"].(string), ",")
	for _, entry := range charsentries {
		if !strings.Contains(entry, "=") {
			continue
		}
		entrykey := strings.Split(entry, "=")[0]
		entryval := strings.Split(entry, "=")[1]
		switch entrykey {
		case "space":
			spacechars = entryval
		case "tab":
			tabchars = entryval
		case "ispace":
			indentspacechars = entryval
		case "itab":
			indenttabchars = entryval
		}
	}

	for ; vloc.Y < w.bufHeight; vloc.Y++ {
		vloc.X = 0

		currentLine := false
		for _, c := range cursors {
			if !c.HasSelection() && bloc.Y == c.Y && w.active {
				currentLine = true
				break

Review comment (suggestion):
The `charsentries` variable is unnecessary, we can directly use `strings.Split()` here. From Go spec (https://go.dev/ref/spec#For_range): "The range expression `x` is evaluated before beginning the loop".

Code after:
	tabsize := util.IntOpt(b.Settings["tabsize"])
	colorcolumn := util.IntOpt(b.Settings["colorcolumn"])

	// this represents the current draw position
	// within the current window
	vloc := buffer.Loc{X: 0, Y: 0}
	if softwrap {
		// the start line may be partially out of the current window
		vloc.Y = -w.StartLine.Row
	}

	// this represents the current draw position in the buffer (char positions)
	bloc := buffer.Loc{X: -1, Y: w.StartLine.Line}

	cursors := b.GetCursors()

	curStyle := config.DefStyle

	// Parse showchars which is in the format of key1=val1,key2=val2,...
	spacechars := " "
	tabchars := b.Settings["indentchar"].(string)
	var indentspacechars string
	var indenttabchars string
	for _, entry := range strings.Split(b.Settings["showchars"].(string), ",") {
		split := strings.SplitN(entry, "=", 2)
		if len(split) < 2 {
			continue
		}
		key, val := split[0], split[1]
		switch key {
		case "space":
			spacechars = val
		case "tab":
			tabchars = val
		case "ispace":
			indentspacechars = val
		case "itab":
			indenttabchars = val
		}
	}

	for ; vloc.Y < w.bufHeight; vloc.Y++ {
		vloc.X = 0

		currentLine := false
		for _, c := range cursors {
			if !c.HasSelection() && bloc.Y == c.Y && w.active {
				currentLine = true
				break
			}
		}
Repository: zyedidia/micro
File: internal/display/bufwindow.go (Go)

Code before:
			for _, mb := range matchingBraces {
				if mb.X == bloc.X && mb.Y == bloc.Y {
					if b.Settings["matchbracestyle"].(string) == "highlight" {
						if s, ok := config.Colorscheme["match-brace"]; ok {
							return r, s, bgoverridable
						} else {
							return r, style.Reverse(true), bgoverridable
						}
					} else {
						return r, style.Underline(true), bgoverridable
					}
				}
			}

			if r != '\t' && r != ' ' {
				return r, style, bgoverridable
			}

			var indentrunes []rune
			switch r {
			case '\t':
				if bloc.X < leadingwsEnd {
					if indenttabchars != "" {
						indentrunes = []rune(indenttabchars)
					} else {
						indentrunes = []rune(tabchars)
					}
				} else {
					indentrunes = []rune(tabchars)
				}
			case ' ':
				if bloc.X%tabsize == 0 && bloc.X < leadingwsEnd {
					if indentspacechars != "" {
						indentrunes = []rune(indentspacechars)
					} else {
						indentrunes = []rune(spacechars)
					}
				} else {
					indentrunes = []rune(spacechars)
				}
			}

			var returnrune rune
			if showoffset < len(indentrunes) {
				returnrune = indentrunes[showoffset]
			} else {
				// use space if no showchars or after we showed showchars
				returnrune = ' '
			}

			if s, ok := config.Colorscheme["indent-char"]; ok {

Review comment (refactor):
Instead of having this `else` branch which duplicates the outer `else` branch below, we can just add `indenttabchars != ""` to the outer `if` condition?

Ditto for `spacechars` below.

Code after:
			for _, mb := range matchingBraces {
				if mb.X == bloc.X && mb.Y == bloc.Y {
					if b.Settings["matchbracestyle"].(string) == "highlight" {
						if s, ok := config.Colorscheme["match-brace"]; ok {
							return r, s, false
						} else {
							return r, style.Reverse(true), false
						}
					} else {
						return r, style.Underline(true), false
					}
				}
			}

			if r != '\t' && r != ' ' {
				return r, style, false
			}

			var indentrunes []rune
			switch r {
			case '\t':
				if bloc.X < leadingwsEnd && indenttabchars != "" {
					indentrunes = []rune(indenttabchars)
				} else {
					indentrunes = []rune(tabchars)
				}
			case ' ':
				if linex%tabsize == 0 && bloc.X < leadingwsEnd && indentspacechars != "" {
					indentrunes = []rune(indentspacechars)
				} else {
					indentrunes = []rune(spacechars)
				}
			}

			var drawrune rune
			if showoffset < len(indentrunes) {
				drawrune = indentrunes[showoffset]
			} else {
				// use space if no showchars or after we showed showchars
				drawrune = ' '
			}

			if s, ok := config.Colorscheme["indent-char"]; ok {
				fg, _, _ := s.Decompose()
				style = style.Foreground(fg)
			}

			preservebg := false
			if b.Settings["hltaberrors"].(bool) && bloc.X < leadingwsEnd {
				if s, ok := config.Colorscheme["tab-error"]; ok {
					if b.Settings["tabstospaces"].(bool) && r == '\t' {