You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/994
-- Commit Summary --
* ui-utils: Load per-version GTK+ CSS file
-- File Changes --
M data/Makefile.am (5) A data/geany-3.0.css (10) A data/geany-3.20.css (8) M data/geany.css (5) M src/ui_utils.c (15)
-- Patch Links --
https://github.com/geany/geany/pull/994.patch https://github.com/geany/geany/pull/994.diff
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994
Don't have 3.20 to test, but LGTM
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994#issuecomment-208686929
I cannot judge about the changes in detail because I just know too less about the GTK3 CSS theming but it fixes some warnings I noticed while testing Geany against GTK 3.20 on Windows.
@sardemff7 it'd be cool if you could update `geany.nsi.in` as well.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994#issuecomment-209102849
Do we assume 3.20 minimum on Windows or not? That makes one (little) file less if we do.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994#issuecomment-209555799
Meh. Doesn't GTK have version blocks in its CSS? just blind hope. Otherwise, I don't like having to do something like that, but LGBI. I'll have to test with 3.20 tho.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994#issuecomment-209639757
Do we assume 3.20 minimum on Windows or not? That makes one (little) file less if we do.
Yes, if GTK3 then 3.20 or newer.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994#issuecomment-209656783
GtkCssProvider *css = gtk_css_provider_new(); GError *error = NULL;
- for ( i = 0 ; i < G_N_ELEMENTS(css_files) && css_files[i].version > gtk_version ; ++i )
- {}
Not a huge deal, but this `for` loop and the above inline struct/variable don't match [Geany's coding style](https://github.com/geany/geany/blob/1.27.0/HACKING#L228). No spaces inside parens or before `;`, braces on newlines, etc).
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994/files/e21492da5b01387cd7a928acecf770...
What is the correct style for the struct?
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994#issuecomment-209804512
@@ -2477,10 +2477,24 @@ void ui_init_builder(void) static void init_custom_style(void) { #if GTK_CHECK_VERSION(3, 0, 0)
- gchar *css_file = g_build_filename(app->datadir, "geany.css", NULL);
- const struct {
guint version;const gchar *file;- } css_files[] = {
{ 20, "geany-3.20.css" },{ 0, "geany-3.0.css" },- };
Use [Allman style](https://en.wikipedia.org/wiki/Indent_style#Allman_style) [braces on their own line](https://github.com/geany/geany/blob/1.27.0/HACKING#L249) instead of [K&R style](https://en.wikipedia.org/wiki/Indent_style#K.26R_style) braces on the same line. That being said, one could argue you're just following [existing code that breaks the convention](https://github.com/geany/geany/blob/1.27.0/src/filetypes.c#L619) :)
In theory it would be like this:
```c const struct { guint version; const gchar *file; } css_files[] = { { 20, "geany-3.20.css" }, { 0, "geany-3.0.css" }, }; ```
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994/files/ca8f6f2094cde6f238f37341df94c5...
@@ -2477,10 +2477,24 @@ void ui_init_builder(void) static void init_custom_style(void) { #if GTK_CHECK_VERSION(3, 0, 0)
- gchar *css_file = g_build_filename(app->datadir, "geany.css", NULL);
- const struct {
guint version;const gchar *file;- } css_files[] = {
{ 20, "geany-3.20.css" },{ 0, "geany-3.0.css" },
It might also be worth putting a comment on these initializers telling that they must be in descending order and that the one with 0 for the `version` must be last, as eluded to in the `while` loop comment below.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994/files/ca8f6f2094cde6f238f37341df94c5...
@@ -2477,10 +2477,24 @@ void ui_init_builder(void) static void init_custom_style(void) { #if GTK_CHECK_VERSION(3, 0, 0)
- gchar *css_file = g_build_filename(app->datadir, "geany.css", NULL);
- const struct {
guint version;const gchar *file;- } css_files[] = {
{ 20, "geany-3.20.css" },{ 0, "geany-3.0.css" },- };
The braces on their own line is certainly the style used in code, but when I looked at other declarations with initializations they seemed to use this style already, maybe I was just (un?)lucky with my samples :)
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994/files/ca8f6f2094cde6f238f37341df94c5...
I can confirm that this change repairs the font on the sci autocomplete popup (without this it uses a proportional font).
Howeer the popup throws lots of assertions: (lt-geany:2407): Gtk-CRITICAL **: gtk_box_gadget_distribute: assertion 'size >= 0' failed in GtkScrollbar
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994#issuecomment-211119787
Well, it's better but still not really nice.
| normal | hover -------------------|--------|------ GTK 3.18 |  |  GTK 3.20 |  |  GTK 3.20 + this PR |  | 
I don't really think it's great as is. Both 3.20 versions get the button too close hiding a part of the label, and none make the button really any kind of smaller. With Adwaita at least it doesn't change the tab's effective height, which is good, but we'd have to see with other themes if we really care. And the version in this PR doesn't seem to get the button really smaller, but removes the border when hovering, making the visual clue worse.
I guess the code here is fine, but the CSS needs a little more love.
I can confirm that this change repairs the font on the sci autocomplete popup (without this it uses a proportional font).
That doesn't make sense as nothing in our CSS touches that. Also, it doesn't fix it for me.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994#issuecomment-212138827
These CSS changes seem to fix it (on Adwaita at least, but although the `margin: -2px` might not be awesome it should work fairly well in all themes): ```diff $ git diff diff --git a/data/geany-3.20.css b/data/geany-3.20.css index 8388818..92bac80 100644 --- a/data/geany-3.20.css +++ b/data/geany-3.20.css @@ -4,5 +4,6 @@ #geany-close-tab-button { outline-offset: 0; outline-width: 0; - border: 0; + margin: -2px; + margin-left: 0; } diff --git a/data/geany.css b/data/geany.css index 2fb7964..5b7f322 100644 --- a/data/geany.css +++ b/data/geany.css @@ -4,12 +4,14 @@ #geany-close-tab-button { padding: 0; } -#geany-close-tab-button GtkImage { +#geany-close-tab-button GtkImage /* GTK < 3.20 */ , +#geany-close-tab-button image /* GTK >= 3.20 */ { padding: 0; }
/* use monospaced font in search entries for easier reading of regexp (#1907117) */ -#GeanyDialogSearch GtkEntry { +#GeanyDialogSearch GtkEntry /* GTK < 3.20 */, +#GeanyDialogSearch entry /* GTK >= 3.20 */ { font-family: monospace; }
``` It's a bit annoying widget names don't work at all anymore, but well.
With Adwaita, this makes the button scale down to about 16px just fine, and it makes it possible to set the `min-height` on `notebook tabs tab` to `0` without the button messing with the sizes -- at no font size the mutton is what limits the tab minimum height.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994#issuecomment-212161379
Could always copy what Gedit does. Last time I looked they seemed to have gotten rid of their GeditSmallButton class, but unless they no longer have a tab close button (wouldn't surprise me), it's likely been fixed there, since it's one of the handful of apps GTK is designed to work for any more.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994#issuecomment-212162565
Does the double-selector work for both GTK+ versions?
Also, why the `margin: -2px;` is needed at all?
Anyway, feel free to squash or follow-up and merge.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994#issuecomment-212294409
Does the double-selector work for both GTK+ versions?
That's a good question. I would imagine it would work on < 3.20 too, but I only tested on 3.20.3 for the moment.
Also, why the `margin: -2px;` is needed at all?
Adwaita has the equivalent to `margin: 4px 4px 4px -4px;` on the button, which makes it take a lot of space, and be too close to the left element. The idea with `-2` was to not take space for the button's borders alike, prefering having it possibly overflow 1 pixel when hovered than taking more place. But that was a great question as it was actually mostly a hack, and you made me investigate further why it was larger than I believed was necessary, and I found out that Adwaita has a `min-width` and `min-height` of about 20 (IIRC) on the button. Resetting that makes the button actually smaller, removing any use for the negative margin.
```diff $ git diff diff --git a/data/geany-3.20.css b/data/geany-3.20.css index 8388818..5cbde30 100644 --- a/data/geany-3.20.css +++ b/data/geany-3.20.css @@ -4,5 +4,8 @@ #geany-close-tab-button { outline-offset: 0; outline-width: 0; - border: 0; + margin: 0; + margin-left: 0.5em; + min-width: 0; + min-height: 0; } diff --git a/data/geany.css b/data/geany.css index 2fb7964..5b7f322 100644 --- a/data/geany.css +++ b/data/geany.css @@ -4,12 +4,14 @@ #geany-close-tab-button { padding: 0; } -#geany-close-tab-button GtkImage { +#geany-close-tab-button GtkImage /* GTK < 3.20 */ , +#geany-close-tab-button image /* GTK >= 3.20 */ { padding: 0; }
/* use monospaced font in search entries for easier reading of regexp (#1907117) */ -#GeanyDialogSearch GtkEntry { +#GeanyDialogSearch GtkEntry /* GTK < 3.20 */, +#GeanyDialogSearch entry /* GTK >= 3.20 */ { font-family: monospace; }
```
With this, I get a prettier small button that scales down very well:  
The `margin-left: 0.5em` is not to get the button collapsed to the label, which I believe is prettier and more readable. Half an `em` seemed like a good visual spacing after a piece of text, but anything else would do.
---- I also played further if we wanna get creative and pretty, to make the buttons on non-active tabs lighter. It's easy enough with the (new?) CSS, but maybe we wanna make sure it's great with more than just Adwaita, although as I just played with the opacity it shouldn't make anything much less nice.
GEdit-ish, always light but on button hover: ```css #geany-close-tab-button { opacity: 0.25; } #geany-close-tab-button:hover { opacity: 1; } ```
light-out only non-active tab, Adwaita-ish: ```css notebook tab #geany-close-tab-button { opacity: 0.25; } notebook tab:hover #geany-close-tab-button { opacity: 0.5; } notebook tab:checked #geany-close-tab-button, notebook tab #geany-close-tab-button:hover { opacity: 1; } ```
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994#issuecomment-212449004
@codebrainz might be interesting indeed, but although I didn't check I wouldn't be surprised they just stopped making it small, as the Adwaita tabs are very tall and the normal-sized button actually fits.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994#issuecomment-212504057
Merged #994.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994#event-648217185
Does the double-selector work for both GTK+ versions?
That's a good question. I would imagine it would work on < 3.20 too, but I only tested on 3.20.3 for the moment.
It does work fine as expected (at least on some earlier versions, but should be fine).
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/994#issuecomment-216385548
github-comments@lists.geany.org