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

use HighContrastInverse theme from gtk+ #210

Merged
merged 1 commit into from
Apr 18, 2018

Conversation

raveit65
Copy link
Member

Since HighContrastInverse is available by gtk+ we don't need to update our fork.
We simply ship the gtk+-2, metacity theme and a index.theme file which use the gtk+-3 part from gtk+.
That's it.

@raveit65 raveit65 requested review from a team April 16, 2018 19:10
@raveit65
Copy link
Member Author

@alexarnaud
Please test.

@lukefromdc
Copy link
Member

I got these errors on running ./autogen.sh --prefix=/usr

Makefile.am: installing './INSTALL'
configure.ac:44: error: required file 'desktop-themes/HighContrastInverse/Makefile.in' not found
configure.ac:44: error: required file 'desktop-themes/HighContrastInverse/gtk-2.0/Makefile.in' not found
configure.ac:44: error: required file 'desktop-themes/HighContrastInverse/gtk-2.0/gtkrc.in' not found
configure.ac:44: error: required file 'desktop-themes/HighContrastInverse/metacity-1/Makefile.in' not found
configure.ac:44: error: required file 'desktop-themes/HighContrastInverse/pixmaps/Makefile.in' not found
desktop-themes/Makefile.am:1: error: required directory desktop-themes/HighContrastInverse does not exist
autoreconf: automake failed with exit status: 1

Copy link
Member

@lukefromdc lukefromdc left a comment

Choose a reason for hiding this comment

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

Build failure, missing desktop-themes/HighContrastInverse directory and files within it

@lukefromdc lukefromdc requested a review from a team April 16, 2018 19:46
@raveit65 raveit65 force-pushed the use-HighContrastInverse-theme branch from 7cf9fdb to 9ebb5cd Compare April 16, 2018 20:07
@raveit65
Copy link
Member Author

Opps, PR is updated.
Try again please.

@lukefromdc lukefromdc self-requested a review April 16, 2018 22:54
Copy link
Member

@lukefromdc lukefromdc left a comment

Choose a reason for hiding this comment

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

Works fine now, was focussed mostly on the panel's appearance. The HighContrast and HighContrastInverse themes come up fine in mate-appearance-properties with this

@monsta
Copy link
Contributor

monsta commented Apr 17, 2018

Looks ok, as I don't use a11y themes anyway.

monsta@asylum:~$ ls -l /usr/share/themes/HighContrastInverse/
итого 32
drwxr-xr-x 2 root root  4096 апр 17 21:48 gtk-2.0
-rw-r--r-- 1 root root 10643 мар 31 13:56 index.theme
drwxr-xr-x 2 root root  4096 апр 17 21:48 metacity-1
drwxr-xr-x 2 root root 12288 апр 17 21:48 pixmaps

Not sure if this could conflict with old GTK+2 and Metacity themes with the same name from GNOME (did such themes exist?).

@monsta
Copy link
Contributor

monsta commented Apr 17, 2018

Also I couldn't find anything from upstream in /usr/share/themes/HighContrastInverse. In which version did they add that theme?

@lukefromdc
Copy link
Member

My current theme directory for HighContrastInverse is empty, and GTK3 does not ship a theme by that name, so I think it's just the HighContrast theme (compiled in the same way as Adwaita rather than in /usr/share/themes) with the colors inverted. Not sure but that's what it looks like. I've always had that theme available, but even back to GTK 3.14 it does not come from a separate theme directory.

@raveit65
Copy link
Member Author

The gtk3 parts of themes from gtk+ are shipped with a gresource file nowadays, imo.
For adwaita, adwaita-dark and HighContrast gnome ships a gtk+-2 part, for this reason they need a folder under /usr/share/themes.
If they decided to create a gtk+-2 theme again with a folder, we can rename our folder quickly and ship only a metacity theme plus a index.theme file.
Same what we did for HighContrast theme.
With gtk+-3.20 i simply copied their theme to our repos to avoid porting our old theme to 3.20 syntax.
Since this time we didn't change anything.
So, our theme isn't really an own theme and we can get rid of it safely.
As a side affect this will reduce the workload.

Copy link
Member

@sc0w sc0w left a comment

Choose a reason for hiding this comment

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

The theme seems to work here with this PR

@monsta
Copy link
Contributor

monsta commented Apr 18, 2018

Hmm, GResource... can mate-appearance-properties load such themes? Either it can't, or I don't have the necessary packages installed - but I thought it should be a part of GTK+ package itself?

I have the following in mate-appearance-properties:

  • Main page:
    High Contrast, High Contrast Inverse

  • Controls:
    HighContrastInverse

  • Window border:
    HighContrast, HighContrastInverse

  • Icons:
    ContrastHigh

Both items on the main page seem to come from our index.theme files:

  • /usr/share/themes/ContrastHigh/index.theme
  • /usr/share/themes/HighContrastInverse/index.theme

Copy link
Contributor

@monsta monsta left a comment

Choose a reason for hiding this comment

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

Ok, if upstream doesn't have GTK+2 and Metacity parts of HighContrastInverse, then this is good to go.

@raveit65
Copy link
Member Author

raveit65 commented Apr 18, 2018

Hmm, GResource... can mate-appearance-properties load such themes?

It can be load if you choose the edit-menu button. In those list you see the themes.
But without having an index.theme file with gtk/metacity/icon-theme the thumbnail won't be created.

@raveit65 raveit65 merged commit 10886a7 into master Apr 18, 2018
@raveit65 raveit65 deleted the use-HighContrastInverse-theme branch April 18, 2018 06:37
@monsta
Copy link
Contributor

monsta commented Apr 18, 2018

Yeah, I know it won't appear on the main page w/o index.theme. But I also don't see them in GTK+ section when I go to details dialog.

@monsta
Copy link
Contributor

monsta commented Apr 18, 2018

Ok, I didn't have gnome-accessibility-themes package installed. So I've installed it... and interesting, it has MetacityTheme set in its index.theme file.

Our theme - /usr/share/themes/ContrastHigh/index.theme:

[X-GNOME-Metatheme]
GtkTheme=HighContrast
IconTheme=Adwaita
MetacityTheme=HighContrast
CursorTheme=Adwaita
CursorSize=24

Their theme - /usr/share/themes/HighContrast/index.theme:

Type=X-GNOME-Metatheme
...
GtkTheme=HighContrast
MetacityTheme=HighContrast
IconTheme=Adwaita
CursorTheme=Adwaita
CursorSize=24

Now I have duplicate themes in thumbnails 🙂

This is gnome-accessibility-themes 3.22.2 in Debian Stretch, so it's not very new. Should we drop our ContrastHigh then?

@monsta
Copy link
Contributor

monsta commented Apr 18, 2018

There was no any HighContrastInverse bits in the package though, maybe it had been added later. I'll check it in more modern systems as well.

@monsta
Copy link
Contributor

monsta commented Apr 18, 2018

Ah I see... they don't have metacity-theme-1.xml, we should keep our own. But what about our index.theme?

@raveit65
Copy link
Member Author

In f26

[root@mother rave]# yum-deprecated provides /usr/share/themes/HighContrast/index.theme

adwaita-gtk2-theme-3.22.3-2.fc26.i686 : Adwaita gtk2 theme
Quelle      : fedora
Übereinstimmung von:
Dateiname     : /usr/share/themes/HighContrast/index.theme

adwaita-gtk2-theme-3.22.3-2.fc26.x86_64 : Adwaita gtk2 theme
Quelle      : fedora
Übereinstimmung von:
Dateiname     : /usr/share/themes/HighContrast/index.theme
[root@mother rave]# cat /usr/share/themes/HighContrast/index.theme
<cut>
Encoding=UTF-8
GtkTheme=HighContrast
IconTheme=Adwaita
CursorTheme=Adwaita
CursorSize=24

I will check it later in f28. We ship the index.theme file in another dir.

[root@mother rave]# cat /usr/share/themes/ContrastHigh/index.theme
<cut>
[X-GNOME-Metatheme]
GtkTheme=HighContrast
IconTheme=Adwaita
MetacityTheme=HighContrast
CursorTheme=Adwaita
CursorSize=24

Of course i am happy to remove our fake dir it it isn't necessary.

@monsta
Copy link
Contributor

monsta commented Apr 18, 2018

Ok, Debian Stretch is too old for this...
https://github.com/GNOME/gnome-themes-extra/commit/0fb555d0bc1b78c7f208e7f4adcf65e35f72cfc3

Looks like we still need to keep it.

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

4 participants