arc-design / arc-theme

A flat theme with transparent elements
GNU General Public License v3.0
696 stars 55 forks source link

Make better use of autotools #149

Closed chewi closed 5 years ago

chewi commented 5 years ago

This is a work in progress so please don't merge it yet. There are a lot of changes so I wanted to run it by you before I do too much more work on it. Nearing the end, mainly compile-gresources.sh still to deal with, though I'm only changing GTK+2, GTK+3, and XFWM4 for now.

jnsh commented 5 years ago

With large PRs like this, it would help greatly if you could split the changes to multiple commits, and explain the change in commit message if it's not obvious. Currently it's extremely difficult to follow what has been done, and more importantly why.

At a quick glance this looks like good work, but I didn't really dig into details at all for the reason above.

chewi commented 5 years ago

Fair enough, I normally would, but it's a bit tricky in this case as a lot of it is inter-related. I'll see what I can. I can at least break it down into the different engines.

Do you guys use make dist? I suspect you do. I had made some of the directories conditional in configure itself so that Makefiles would not be created but I now understand that this breaks make dist.

jnsh commented 5 years ago

I can definitely have a better look and give feedback either way, but the cleaner you can make it, the better.

In any case, could you give a little more detailed description of what this PR does. I've read some of your goals from #26, but I'm a bit uncertain what's been implemented here. Looks like you've at least moved the asset rendering from the render-assets scripts to Makefiles (which is very nice), but some of the changes seem unrelated to that?

I'm don't know about make dist. Better check that from @NicoHood

NicoHood commented 5 years ago

What is make dist?

jnsh commented 5 years ago

@NicoHood I've never had to use it so I'm not 100% sure, but IIUC make dist is meant to be used for making tarballs for a release. See the documentation for dist target in https://www.gnu.org/prep/standards/html_node/Standard-Targets.html

chewi commented 5 years ago

That's right. I thought you were using it because you already have EXTRA_DIST in one or two places. Even if you don't use it, I don't like the idea of breaking it so I've made the changes and got it working. More later.

chewi commented 5 years ago

I've split this out a bit now, hopefully that makes things sufficiently clear.

make dist works now although it doesn't preserve the symlinks to _colors.scss.thpl. Close enough.

Although I've been checking it puts the files in the right places, I haven't actually tried it out for real yet so I'll give it a road test before declaring it ready.

chewi commented 5 years ago

Hmm, not so good at the moment. The Dark theme is not looking very dark under GTK+3! It works under GTK+2 though. I thought perhaps the ../assets paths in the XML were breaking it but I changed this by using a symlink and it made no difference. I'll keep digging.

chewi commented 5 years ago

Figured it out. I had the Dark theme pointing to gtk-darker.css instead of gtk-dark.css. I think this is good to go now. I look forward to your feedback.

fossfreedom commented 5 years ago

I've run through quite a few compilation options and this PR works quite nicely.

Excellent work @chewi

The --with-gtk3 option is broken for version 3.28/3.30 - but this is an existing issue - nothing to do with this PR.

Thanks

jnsh commented 5 years ago

Well, I was still on the process of going trough the code on the last few commits, and didn't want to comment until I was done. There were a few things that I think should've been added to complete this PR, and a few small things that I would have liked to ask about before merging.

Most notably, the changes in Set GTK3_VERSION to compatible version instead of actual version should have been made similarily to other versioned themes (i.e. the gnome-shell theme) as well. This is for consistency between the versioned themes, and because it is a nice improvement.

Additionally, the instructions in HACKING should be updated according to the chabes in asset rendering, and it would be nice if README mentioned about which theme ports actually require sassc, inkscape or optipng. Some of the error messages could have been made clearer as well

Rest of my notes were about possibly making the new file hierarchies and coding style clearer on some areas. All of the above can be added later, but I thought I'd share my feedback anyway, since I went trough most of the code.

One thing that should be noted though, that I'm fairly sure this PR breaks the oomox support.

If the intention is to get the "Next Release" out soon, personally I would have not merged such a major PR until for the "Next Next Release", just so that any possible issues can be ruled out before shipping this as stable to distributions. @fossfreedom @NicoHood, please take note of this, especially considering the thing about oomox that I mentioned above.

Nevertheless, this PR is really nice work, and greatly improves the build system and helps further development. Thanks for the effort @chewi

fossfreedom commented 5 years ago

From a major distribution point of view fedora has maintainer issues so they are not yet using this repo.

Agree and disagree. I did take into account the distro situation.

Debian will not freeze until end of Jan ... so further testing feedback is useful.

Ubuntu will not freeze until March.

There are no other major fixed release major distros. Arch and Manjaro are rolling releases and Nico can manage fixes.

Will go for a release now.

Nico I would like to plan for an end of year begin Jan release to meet debian Ubuntu and maybe fedora freeze dates.

jnsh commented 5 years ago

Did you account the fact that this breaks oomox support as I mentioned? If it's OK to break, why do we even support it in the first place?

Furthermore, this PR doesn't provide any real improvements to the build system, but is basically just cleanups and infrastructure improvements. I don't think that justifies rolling out a release that breaks something else within the theme.

IMHO, either the oomox support should be fixed before the release, or this PR could be postponed to properly polish and fix any issues with it for the future releases.

fossfreedom commented 5 years ago

Release now. Can resolve subsequently. If we don't release now then this PR will not be available to Debian until July next year and Ubuntu until May.

I will not allow major changes close to freeze dates.

chewi commented 5 years ago

I noticed the change_color.sh script but didn't understand its significance. More importantly, I've noticed the window buttons on applications like gthumb and evince are broken. The XFWM stuff is quite simple but these possibly don't use normal window decorations? If you could wait an hour or so, I'll try and get all this resolved.

NicoHood commented 5 years ago

All issues for the next release are closed. If nobody complains I will push out a new release tomorrow. If there is still something wrong with it, I have no problem is pushing another release within the next weeks for quick fixes.

fossfreedom commented 5 years ago

Thanks Nico. Good job from everyone who contributed. This is a big release. IMHO more than a minor point release. But I will leave that to you.

NicoHood commented 5 years ago

We do not have major or minor releases, as the date is used as version number. But its quite major, yes.

fossfreedom commented 5 years ago

Sure. I was referring to the release description. Sorry for the confusion