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

Themable compiler output in message window #1377

Closed
wants to merge 2 commits into from
Closed

Themable compiler output in message window #1377

wants to merge 2 commits into from

Conversation

Rakksor
Copy link

@Rakksor Rakksor commented Jan 25, 2017

Adds four new styling options to customize the colors in the compiler tab to address issue #1376. When the values are not set by a colorscheme the previously hardcoded colors will be used.

A minor drawback is that the few lines in the compiler tab that were using the GTK default foreground color don't do that anymore and are by default black now (but also themable).

@codebrainz
Copy link
Member

+1 for unhardcoding these colours, but I think it would be better to use GTK+ theming for this. The colour schemes are only supposed to affect the Scintilla editor widget. In other places where we affect the other widgets (ex. tab label colour, search entry background, etc) we already use GTK+ themes. It may even be possible to re-use the colours from GTK+ itself, like using the warning, error, info, and other colours used by GtkInfoBar, so that native dark or light GTK+ themes "just work" out of the box without having to futz with a whole other colour scheme system.

@b4n
Copy link
Member

b4n commented Jan 25, 2017

I agree with @codebrainz. It might be harder to do than e.g. the tab labels, because I don't think it's possible to set names/classes on treeveiw cells, but it's likely possible to still query the theme.

As @codebrainz mentioned, it makes it possible to play nice with the theme, and even if we didn't re-use some style that already exists (even though it'd be likely better if possible), because a theme could (theoretically) provide the Geany-specific bits itself to blend perfectly.

@elextr
Copy link
Member

elextr commented Jan 26, 2017

Don't agree with using the theme (at least as proposed above).

While at first glance it might seem nice to have the themes provide the styling, in reality they won't provide a Geany-compiler-error-colour or equivalent. So we would have to use standard colours if it was to follow the theme.

Unfortunately standard warning and error colours are, as @codebrainz said, designed for large slabs of colour, and not for fine text, so they may not work in that context. In fact my dark theme sets warning colour to black, NSFW for dark and warning you say, but it then changes the background to a lighter warning or error colour, thus showing it in a larger slab. So Geany would have to be changed to set both the colour and the row background in the treeview rows to use this in Geany, although it will be pretty intrusive on a dark theme to have brightly coloured row backgrounds in the compiler window..

So the user is left having to learn how to edit a local modification to themes in either GTKrc or GTK-not-quite-really-CSS, depending on the version of GTK in use, instead of a file format controlled by Geany and so not likely to change.

I can confirm the problem, having only recently switched to a dark theme and the distro being Mint, its greenish, so red text is difficult to read against the background (and my colour acuity is (just) classified as safe, pity the 10% of males with real colour blindness).

So since it is a real problem (I have it 😄 ) I propose that we wait for a bit and see if a viable themes based pull request emerges, if not we commit this fairly simple solution as an interim (assuming it works, not tested yet) with an issue to change it to using themes.

@Rakksor
Copy link
Author

Rakksor commented Jan 27, 2017

I actually didn't realize that there was already the option to customize some colors using the GTK theme, which is why I did it using colorschemes.

I agree that using the GTK theme would be better, as it is more consistent with how geany works at the moment (-> as codebrainz pointed out: colorschemes for the editor, gtkrc for the other widgets). While it is true that no theme will provide the colors for those specific styles, this is also the case for all the other widgets that are at styled using geany.gtkrc (tab foregrounds, etc).

Additionally, the current implementation only changes the foreground color according to the colorscheme, whereas the background color is still defined by GTK, meaning that using a dark colorscheme and a light GTK theme or vice versa will again lead to unreadable messages.

I changed the fix to load the colors from the GTK theme, should I make a new pull request for it or update this one? The new version does not reuse the GTK colors for error, warning and info (I'm not sure they exist in GTK2), but the current default colors are defined in geany.gtkrc and can then be changed in the same way as for example the tab colors.

@elextr
Copy link
Member

elextr commented Jan 27, 2017

Since there is a disagreement about which is the right way to do it, probably better to make a separate PR so they can be compared.

@elextr
Copy link
Member

elextr commented Jan 27, 2017

Note, see #1380 for a pointer to how notebook tab colours are "themed".

@codebrainz
Copy link
Member

See #1382

@Rakksor
Copy link
Author

Rakksor commented Jan 29, 2017

Is it OK if I close this version? I was a bit stupid and used the master branch for it, now I can't make new branches without these changes.

@elextr
Copy link
Member

elextr commented Jan 29, 2017

@Rakksor sure, its your pull request, but I'll push the close and comment for you.

@elextr elextr closed this Jan 29, 2017
@Rakksor
Copy link
Author

Rakksor commented Jan 29, 2017

Thanks 😀

@codebrainz
Copy link
Member

Superseded by #1382 which includes the important part of this PR (b53143a).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants