Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Theme to a DropDown with flyout #433

Merged
merged 7 commits into from
Jul 8, 2024
Merged

Conversation

Natestah
Copy link
Contributor

  • Little bit of MVVP in here to enable bindings in the AXAML
  • Drop down, allows me to look at specific themes, As I intend to leverage a little more of the info from the TextMateSharp JSON's

HELLO, I'm the author of BlitzSearch ( https://natestah.com ). I'm making heavy use of AvaloniaEdit and there are things that I want to do with it. This would be a small Ice Breaker change, would be awesome if the efforts could align.

2024-06-20_07-42-39

- Little bit of MVVP in here to enable bindings in the AXAML
- Drop down, allows me to look at specific themes, As I intend to leverage a little more of the info from the TextMateSharp JSON's

HELLO, I'm the author of BlitzSearch ( https://natestah.com ).  I'm making heavy use of AvaloniaEdit and there are things that I want to do with it.  This would be a small Ice Breaker change, would be awesome if the efforts could align.
-Added a applywindowProperties bool, so that people can opt out ( please review, this probably should default to off or just not exist here )
-Some  int->FontStyle reference that came up when I upgraded TextMateSharp
-In Demo AXAML's remove "Foreground" setters so that theme can do it's thing on the window.
-undoing the direct project reference.
-looks like I fixed up a defaulting the line number to the foreground color change
@jmacato jmacato enabled auto-merge July 2, 2024 02:53
src/AvaloniaEdit.TextMate/TextMate.cs Outdated Show resolved Hide resolved
src/AvaloniaEdit.TextMate/TextMate.cs Outdated Show resolved Hide resolved
src/AvaloniaEdit.TextMate/TextMate.cs Outdated Show resolved Hide resolved
-Remove apply window updates bool.
-Remove duplicated CurrentHighlightRenderer Background/border settings, this is replaced with a new call for but doesn't answer the review note fully.
auto-merge was automatically disabled July 4, 2024 13:54

Head branch was pushed to by a user without write access

@danipen
Copy link
Collaborator

danipen commented Jul 4, 2024

@Natestah I tried the changes and found some issues:

  • Once you open the demo window for the first time, the selection is not visible or it doesn't work.
  • After changing the theme, the selection becomes visible.
  • After changing the theme again, the selection color doesn't properly changes.

See the attached video for clarification:

testing.mov

-fallback to the Resource defined Selection brush rather than the first seen when the TextMate Defs don't have a selection background.
@Natestah
Copy link
Contributor Author

Natestah commented Jul 4, 2024

I think Everything is up-to-date, thank you for time and patience. I'm still learning how to use GitHub and Avalonia

Copy link
Member

@maxkatz6 maxkatz6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you.

@maxkatz6
Copy link
Member

maxkatz6 commented Jul 4, 2024

cc @danipen if you want to have another look

@danipen
Copy link
Collaborator

danipen commented Jul 5, 2024

I'm rethinking this a bit.

We should let users use this functionality, but it shouldn't be mandatory. Some customers likely have custom background or selection colors, so we'd be breaking their styles. Setting styles for the text editor should be the user's responsibility.

The TextMate API should be responsible of notifying about theme changes and providing the theme colors. Let me rethink this a bit and I'll do a small refactor.

 We should let users use this functionality, but it shouldn't be mandatory. Some customers likely have custom background or selection colors, so we'd be breaking their styles. Setting styles for the text editor should be the user's responsibility. The TextMate API should only notify about theme changes and provide theme colors.
@@ -18,7 +18,7 @@ public class ForegroundTextTransformation : TextTransformation
public Action<Exception> ExceptionHandler { get; set; }
public int ForegroundColor { get; set; }
public int BackgroundColor { get; set; }
public int FontStyle { get; set; }
public FontStyle FontStyle { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't noticed before, but changing public property type is an ABI breaking change. Might not be critical, but needs to be carefully considered.

Copy link
Collaborator

@danipen danipen Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this API break comes from TextMateSharp ...

@danipen danipen merged commit 318277d into AvaloniaUI:master Jul 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants