oligo / gioview

Gio-view: A Toolkit for Faster Gio App Development
MIT License
72 stars 8 forks source link

Editor syntax highlighting example panics on a bad regular expression pattern. #15

Closed bogen85 closed 1 month ago

bogen85 commented 2 months ago

The following makes it not panic, but it is not an ideal fix. https://github.com/bogen85/gioview/commit/26fc3cc3a57c9d2387c1cd340ec61b845f7b679c

Basically just called regex.Compile and checked for and logged the error instead of calling regex.MustCompile.

Since error/pattern state is not being tracked, logging of error repeats until the pattern is corrected.

Oh, by the way, kind of a separate issue, but the 2 files had different white-space settings from your repo, one with spaces, one with tabs, so go fmt broke things and had to tell my editor the differences from the Go defaults. (Which is tabs, not spaces). The inconsistencies are going to make pull requests harder.

Want to discuss how the error should be handled for your example (having the example panic is not ideal) before I make a PR.

bogen85 commented 2 months ago

I guess no need to keep track of state as this is just refreshed. So I just added a status label above the "Regex of substring hightlighted" caption. https://github.com/bogen85/gioview/commit/9e7267df2a8024412712588f719979451a4f7ee8

image

It should likely be red or something, but I'm new to gio and gioview.

bogen85 commented 2 months ago

I'm also new to immediate mode GUI programming. I know this is just an example I've created an issue on, but it is good example for learning on to use Gio-view (and Gio in general).

Alright, so for my fix, theme is being passed in which is where the color is coming from.

// status label, need to make it red for an error?
layout.Rigid(func(gtx C) D {
    return material.Label(th.Theme, th.TextSize, errString).Layout(gtx)
}),

Since Theme has a registration mapping, when the theme is created, would it be appropriate to register sub-themes as well for error status and other informative status?

oligo commented 2 months ago

The following makes it not panic, but it is not an ideal fix. bogen85@26fc3cc

Basically just called regex.Compile and checked for and logged the error instead of calling regex.MustCompile.

Since error/pattern state is not being tracked, logging of error repeats until the pattern is corrected.

Oh, by the way, kind of a separate issue, but the 2 files had different white-space settings from your repo, one with spaces, one with tabs, so go fmt broke things and had to tell my editor the differences from the Go defaults. (Which is tabs, not spaces). The inconsistencies are going to make pull requests harder.

Want to discuss how the error should be handled for your example (having the example panic is not ideal) before I make a PR.

Thank you for reporting the two issues. I will fix the indentation issue later. Someone reported the RE panic issue in another thread, and I explained that this is just a demo purpose example, so not every corner case would be covered. I think I missed one thing that maybe others do not know how to process the error here. So hope I can help you figure it out here.

oligo commented 2 months ago

I guess no need to keep track of state as this is just refreshed. So I just added a status label above the "Regex of substring hightlighted" caption. bogen85@9e7267d

image

It should likely be red or something, but I'm new to gio and gioview.

For general purpose error handling, I prefer to use an error field tracking the failure, and reset it when related info updated.

And in the case of this example, I would suggest using the Input widget to highlight the error. It exposed setError and clearError APIs to help you handle the exceptions.

oligo commented 2 months ago

I'm also new to immediate mode GUI programming. I know this is just an example I've created an issue on, but it is good example for learning on to use Gio-view (and Gio in general).

Alright, so for my fix, theme is being passed in which is where the color is coming from.

// status label, need to make it red for an error?
layout.Rigid(func(gtx C) D {
    return material.Label(th.Theme, th.TextSize, errString).Layout(gtx)
}),

Since Theme has a registration mapping, when the theme is created, would it be appropriate to register sub-themes as well for error status and other informative status?

Yes, that's the desired way to extend your theme.

bogen85 commented 2 months ago

For general purpose error handling, I prefer to use an error field tracking the failure, and reset it when related info updated.

And in the case of this example, I would suggest using the Input widget to highlight the error. It exposed setError and clearError APIs to help you handle the exceptions.

Thanks! Yes, this works as you suggested (better than introducing a new status label):

layout.Rigid(func(gtx C) D {
    vw.patternInput.Padding = unit.Dp(8)
    vw.patternInput.HelperText = "Illustrating colored text painting in text editor."
    vw.patternInput.MaxChars = 64
    if errString == "" {
        vw.patternInput.ClearError()
    } else {
        vw.patternInput.SetError(errString)
    }
    return vw.patternInput.Layout(gtx, th, "Regex of substring hightlighted")
}),

image

oligo commented 2 months ago

Yes, that's it. I pushed a commit showing the error handling as you demonstrated, and fixed the indentation issue.

bogen85 commented 2 months ago

Yes, that's it. I pushed a commit showing the error handling as you demonstrated, and fixed the indentation issue.

Yeah, but the panic still occurs due to using regexp.MustCompile rather than regexp.Compile So yeah, the code now has an example of the error handling, but it is not visualized due to the panic.

func stylingText(text string, pattern string) []*editor.TextStyle {
    var styles []*editor.TextStyle

    re := regexp.MustCompile(pattern)
    matches := re.FindAllIndex([]byte(text), -1)
    for _, match := range matches {
        styles = append(styles, &editor.TextStyle{
            Start:      match[0],
            End:        match[1],
            Color:      colorToOp(color.NRGBA{R: 255, A: 212}),
            Background: colorToOp(color.NRGBA{R: 215, G: 215, B: 215, A: 100}),
        })
    }

    return styles
}

It is your example. But the panic caused to dig into the code and figure out how this whole "Immediate GUI" thing works and how I can use Gio (and Gio-view) in new programs. So thanks for it having a bug I could figure out how to fix!

bogen85 commented 2 months ago

Yes, that's it. I pushed a commit showing the error handling as you demonstrated, and fixed the indentation issue.

Thanks for correcting the indentation issue. That was confusing for there to be a tab white-space mismatch for same language source files in the same directory.

bogen85 commented 2 months ago

Since Theme has a registration mapping, when the theme is created, would it be appropriate to register sub-themes as well for error status and other informative status?

Yes, that's the desired way to extend your theme.

On the issue of themes, in theory how hard would it be to hook into existing theming systems, such as from a Gtk or Qt environment, and update Gio-view themes based on data extracted from them, that way apps using Gio-view could have a consistent look and feel to other native apps on one's system.

What I see in gioview/theme looks like what is the system theme could be used to update the applicable areas in the app UI's theme.

I'm not making a feature request, I just want to know your thoughts on this.

oligo commented 2 months ago

Yes, that's it. I pushed a commit showing the error handling as you demonstrated, and fixed the indentation issue.

Yeah, but the panic still occurs due to using regexp.MustCompile rather than regexp.Compile So yeah, the code now has an example of the error handling, but it is not visualized due to the panic.

func stylingText(text string, pattern string) []*editor.TextStyle {
  var styles []*editor.TextStyle

  re := regexp.MustCompile(pattern)
  matches := re.FindAllIndex([]byte(text), -1)
  for _, match := range matches {
      styles = append(styles, &editor.TextStyle{
          Start:      match[0],
          End:        match[1],
          Color:      colorToOp(color.NRGBA{R: 255, A: 212}),
          Background: colorToOp(color.NRGBA{R: 215, G: 215, B: 215, A: 100}),
      })
  }

  return styles
}

It is your example. But the panic caused to dig into the code and figure out how this whole "Immediate GUI" thing works and how I can use Gio (and Gio-view) in new programs. So thanks for it having a bug I could figure out how to fix!

Yes the commit does not fix the panic. A better way is to handle this kind of check in the Input widget's validation callback, but for now it does not have a validation hook mechanism. I'm glad to be able to help you figure it out.

oligo commented 2 months ago

Since Theme has a registration mapping, when the theme is created, would it be appropriate to register sub-themes as well for error status and other informative status?

Yes, that's the desired way to extend your theme.

On the issue of themes, in theory how hard would it be to hook into existing theming systems, such as from a Gtk or Qt environment, and update Gio-view themes based on data extracted from them, that way apps using Gio-view could have a consistent look and feel to other native apps on one's system.

What I see in gioview/theme looks like what is the system theme could be used to update the applicable areas in the app UI's theme.

I'm not making a feature request, I just want to know your thoughts on this.

Hooking into existing theming systems should be not too hard as far as I know. GTK or Qt, and maybe OS such as Windows and MacOS has APIs/commands to retrieve styling configs. For Gtk we have gsettings, and for KDE/Qt we have KConfig. The remaining work will be doing some proper mapping with the theme of gioview/gio. This sounds a valuable point. Thanks for pointing it out.

bogen85 commented 2 months ago

Hooking into existing theming systems should be not too hard as far as I know. GTK or Qt, and maybe OS such as Windows and MacOS has APIs/commands to retrieve styling configs. For Gtk we have gsettings, and for KDE/Qt we have KConfig. The remaining work will be doing some proper mapping with the theme of gioview/gio. This sounds a valuable point. Thanks for pointing it out.

KConfig is not the only one for Qt. The environment variable for QT_QPA_PLATFORMTHEME can be set to kde, qt5ct or qt6ct. Not all Qt environments have a full KDE configuration setup installed and for them using qt5ct or qt6ct is much easier to manage.

oligo commented 2 months ago

KConfig is not the only one for Qt. The environment variable for QT_QPA_PLATFORMTHEME can be set to kde, qt5ct or qt6ct. Not all Qt environments have a full KDE configuration setup installed and for them using qt5ct or qt6ct is much easier to manage.

So the situation becomes a little bit complex:

  1. If the app is running in Windows or MacOS, gioview theme aligns with system theme.
  2. For *nix systems, detecting the desktop environments by checking XDG_CURRENT_DESKTOP, to check If Gnome or KDE (or another ones, maybe) is used, we query the system tool for such style settings.
  3. And if we are desired to target a specific theming system, we check the specific variable such as QT_QPA_PLATFORMTHEME as you pointed out.

Anyway there would be lots of source adapters here, and the above thoughts should be feasible. If no automatic hooking is needed, developers can bring their own source adapter.