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 editor-color (#67) #99

Merged
merged 15 commits into from
Mar 17, 2021
Merged

Conversation

XLPhere
Copy link
Contributor

@XLPhere XLPhere commented Jan 6, 2021

Hi,
This should address issue #67
It will add the option to change the background-color, the default-foreground-color, text-selection-color, caret-color and the background of the highlighted line. This will only touch the non-generic editor (JEditBasedTextArea), which also has the other color-options.

Status:

  • Adding the option to the internal settings
  • Option in the settings menu to change the color

This is my first time adding something to a java-ui so please go ahead and tell me if there is any issue.
~ XLP

(This fixes #67)

@XLPhere
Copy link
Contributor Author

XLPhere commented Jan 6, 2021

I'll probably get to implementing the menu-part in the next few days.

@XLPhere XLPhere changed the title WIP: Change editor-color (#67) Change editor-color (#67) Jan 7, 2021
@XLPhere XLPhere marked this pull request as ready for review January 7, 2021 16:06
@TheThirdOne
Copy link
Owner

Thanks for making a PR for this.

It would be good to have the new color selectors in a box like the "Syntax Styling" box. Currently they look very squished in the bottom left. Additionally, it is currently missing an easy way to reset to defaults.

It would be nice if the line numbers respected the settings. I really have no idea how hard that might be, but currently it is quite weird to have the line numbers stick out.

@XLPhere
Copy link
Contributor Author

XLPhere commented Jan 8, 2021

It would be nice if the line numbers respected the settings. I really have no idea how hard that might be, but currently it is quite weird to have the line numbers stick out.

Done!

It would be good to have the new color selectors in a box like the "Syntax Styling" box. Currently they look very squished in the bottom left. Additionally, it is currently missing an easy way to reset to defaults.

The reason why there are down there, has because syntax styles are managed differently compared to the other color settings used in the UI. But sure i can try to put them into a similar box.

On the reset functionality, the reset should work with global reset button already. But I'm currently working on a way to optionally also use the look and feel as a default-option.

@TheThirdOne
Copy link
Owner

The reason why there are down there, has because syntax styles are managed differently compared to the other color settings used in the UI. But sure i can try to put them into a similar box.

Yeah, it definitely makes sense to not include them in the "Syntax Styling" box. A separate, but similar box would be ideal.

On the reset functionality, the reset should work with global reset button already. But I'm currently working on a way to optionally also use the look and feel as a default-option.

I hadn't tested the global reset. That should be fine then.

@XLPhere
Copy link
Contributor Author

XLPhere commented Jan 8, 2021

I'm looking into it - Also experimenting, if we could also make use of the colors provided by the system via look and feel

@XLPhere
Copy link
Contributor Author

XLPhere commented Jan 8, 2021

I'm aware that e086eea is rather big, but it should enable the system-colors to be used, if wished.
The reason why i think that useful, is that on packaged distributions (like mine from the arch-aur (aur/rars)) the script starts the jar with the option -Dswing.defaultlaf=com.sun.java.swing.plaf.gtk.GTKLookAndFeel, which makes the app use the colors/styles of the system for the components (of course only there where it hasn't been changed manually).
This commit should enable the UI to use those system-colors, options to take the default color or a custom colors are also still available.
The default color should also be used, if the system color can not be found and no custom color has been set.

@XLPhere
Copy link
Contributor Author

XLPhere commented Jan 8, 2021

The layout hasn't really changed yet, I'll do that next

@XLPhere
Copy link
Contributor Author

XLPhere commented Jan 8, 2021

Okay the settings should be also now relocated

@XLPhere
Copy link
Contributor Author

XLPhere commented Jan 11, 2021

@TheThirdOne is there anything still to do or is this good now?

@TheThirdOne
Copy link
Owner

I did not notice your last commit so I thought that the relocation has yet to be implemented. Sorry about that.

I will review your PR fully sometime within a week. Thanks for taking the time to make it.

@TheThirdOne
Copy link
Owner

I found three issues during my review. Only the first will delay merging this, but I am content to fix it so you don't need to take any action unless you want this merged a little faster. I am including the others here mostly as a note to myself in the future.

System line highlight being derived from background and line selection is not intuitive from the UI. At first I had thought that there was a bug in saving the preference for background color; making the color displayed update when you change either of the mixed colors would be essential.

On my machine, using the gtk look and feel makes the custom colors not appear properly. I suspect this is not a universal problem and I also believe this is a preexisting issue as it also affects the syntax highlighting colors.

I don't think that the old code for backgrounds of the editer actually ever made it visibly gray, but I would like to be more sure.

@XLPhere
Copy link
Contributor Author

XLPhere commented Jan 17, 2021

On 1: Yes i kinda looked for a neat way to get a system-color for the line-selection, since there is no property in the look and feel for it. So i thought i would be a good idea to mix the selection and background to automatically get some kind sensible color scheme there.
Then i also thought if i already mix the both colors from the color-section, would be a good idea, since it would automatically also come up with a matching color when you manually change the background, so you don't have to mix it yourself. But yes it is definitely not intuitive from a UI standpoint.
Also the reason that it only updates the mixed colors on the apply, is that the mix-color is generated from the colors of the saved config and i don't think there is a simple way to preview it without making the code way more complex just for that case.
But sure i can build it back to it simply just mixing two look-and-feel colors, so it won't have any reference to any other setting and therefore will also always preview accurately.

On 2: Yes i have a similar experience in the settings, the color-buttons don't have colors there - the reason is that in gtk the buttons probably may not have colors. The rest of the app (besides some smaller unimportant details) is themed as excepted though.

On 3: Yes it indeed didn't change anything, since it only updated the Panel iirc, but didn't update the painter (which has a separate background-setting)

@XLPhere
Copy link
Contributor Author

XLPhere commented Feb 1, 2021

Hi, sorry for the late continuation of this.
Here is the "fix" for the system color of line-highlight not being that intuitive.
I am now mixing directly from the system-colors instead of the settings, this might now be less practical, but it's definitely more intuitive. I left the old color-mix option in there in case you want to go back to the setting-mixing.
Let me know if there is anything left to address

@TheThirdOne TheThirdOne merged commit 2eb7c66 into TheThirdOne:master Mar 17, 2021
@TheThirdOne
Copy link
Owner

Sorry that it took so long for me to merge this in. I haven't worked on RARS much recently.

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.

Add background to syntax colors
2 participants