micro icon indicating copy to clipboard operation
micro copied to clipboard

Duplicat colorsheme in status bar when user has custom colorscheme with similar the same name

Open dustdfg opened this issue 1 year ago • 7 comments

Description of the problem or steps to reproduce

  1. Ctrl+e
  2. > set colorscheme + Tab (to see all the schemes in status line)
  3. See two duplicates

screen-1701331767

You can select any of them and will load user's custom implementation. If you just manually type set colorscheme <scheme>, it will also select users color scheme

Specifications

Commit hash: v2.0.13 https://github.com/zyedidia/micro/commit/68d88b571de6dca9fb8f03e2a3caafa2287c38d4 OS: Debian 12 Terminal: foot

dustdfg avatar Nov 30 '23 09:11 dustdfg

Note: I almost created PR so I will soon create it or upload the patch in this issue if I will have troubles

dustdfg avatar Nov 30 '23 09:11 dustdfg

OK. I have a solution but is partial and breaks other things. The commit showed by github above this message de-duplicates solves this problem but make worse another problem with reload #3059. If the commit is applied, the reload will create two duplicates instead of one (except files that have redefinition in user config dir :/)

dustdfg avatar Nov 30 '23 10:11 dustdfg

From my perspective user defined color schemes should take precedence in the moment they have the same name. So the easiest way is to add them first and while adding the builtin we should check if there is already an entry with the same name and thus skip the current. In #3059 I confirmed the bug.

JoeKar avatar Nov 30 '23 17:11 JoeKar

0_o current implementation rtfiles.go#L164 already loads user defined colorschemes at first and only than load builtins but the function that adds builtins doesn't check user defined colorscheme was already loaded. My commit do exactly it: adds check if user defined function was already loaded

dustdfg avatar Nov 30 '23 18:11 dustdfg

Correct me if I'm wrong, but your commit adds it as long as the asset file name is != to the current checked user defined one. Furthermore it doesn't respect the pattern.

How about the following more generic approach?

func AddRuntimeFilesFromAssets(fileType RTFiletype, directory, pattern string) {
	files, err := rt.AssetDir(directory)
	if err != nil {
		return
	}

assetLoop:
	for _, f := range files {
		if ok, _ := path.Match(pattern, f); ok {
			for _, rf := range realFiles[fileType] {
				if f == rf.Name() + strings.TrimPrefix(pattern, "*") {
					continue assetLoop
				}
			}
			AddRuntimeFile(fileType, assetFile(path.Join(directory, f)))
		}
	}
}

JoeKar avatar Nov 30 '23 21:11 JoeKar

Correct me if I'm wrong, but your commit adds it as long as the asset file name is != to the current checked user defined one.

Yes my code is a piece of ~~s**t~~ spaghetti :) Approach without negation of check is better and more convenient and easy to understand

Yes. It adds the embedded file only if it's name not equal to the name of user loaded file. It is your aim not to load duplicates. Your approach is to skip file when the names are equal. Your code won't skip when names different. My code won't add when filenames are equal. It is in general the same thing but yes as stated above approach without negation is more convenient and easy to understand

Furthermore it doesn't respect the pattern.

You are absolutely right

How about the following more generic approach?

It is much better than my. No spaghetti. I think it is ready for PR :)

It even avoids the pitfall of the check if len(realFiles[fileType]) > 0 { if doesn't have any realFile. Non existence of which caused a bug: when you don't have realFile, you won't load any embedded file of appropriate file type (in my non generic case only color schemes).

And doesn't have any influence to reload which will just create 1 duplicate as expected...

Thank you

dustdfg avatar Dec 01 '23 06:12 dustdfg

I also found an issue https://github.com/zyedidia/micro/issues/2795 where the problem was described before this issue so it also should be closed after merging the PR

dustdfg avatar Dec 12 '23 12:12 dustdfg