search

lanoxx / tilda

A Gtk based drop down terminal for Linux and Unix
GNU General Public License v2.0
1.28k stars 161 forks source link

port Tilda VTE 2.90 ->2.91 #138

Closed pik closed 9 years ago

pik commented 9 years ago

Fixed Port (I hope) for Tilda to function correctly with VTE2.91 (based on @x2b's patch). #94

The following are gone from VTE and have been removed from Tilda accordingly:

The following have changed with VTE2.91:

The following have also been updated:

@lanoxx This probably would have been better for history if I had committed each notable change separately - sorry about that. Not sure if now it's better to rebase into one commit, or to go back and split the first commit into at least two or three.

pik commented 9 years ago

Updated VTE-port for backwards compatibility (sorry about the force push, github doesn't let you switch the branch a pull request points to, old patch here for reference: https://github.com/pik/tilda/tree/vteport_old).

Changes for VTE2.91

edit: Had an issue with transparency not working with a background image - but that seems to be the same with the master branch, if it's worth fixing? (Not sure since it's deprecated in 2.91 anyways, I can open an issue for it).

edit2: fixed typo.

lanoxx commented 9 years ago

In my opinion you can always do a forced push, when you updated or rebased your code. So far I have not had problem with that. But, please keep different bug fixes and features in separate branches and separate pull requests (that makes it much easier for me to review and merge things into master.

About the word_char feature, it looks like this is coming back in VTE 0.40.2: https://developer.gnome.org/vte/0.40/VteTerminal.html#vte-terminal-set-word-char-exceptions, so I would prefer to keep this option, or only remove it for the versions that do not support it.

pik commented 9 years ago

In my opinion you can always do a forced push, when you updated or rebased your code. So far I have not had problem with that. But, please keep different bug fixes and features in separate branches and separate pull requests

Not sure what you mean about the forced push - I opened an issue for dealing with deprecated config options / libconfuse though. #161

Yes, word-chars should be versioned and re-enabled if possible, I'll try and take another look over this PR later this week.

lanoxx commented 9 years ago

I was refering to this comment of yours:

Updated VTE-port for backwards compatibility (sorry about the force push, github doesn't let you switch the branch a pull request points to, old patch here for reference: https://github.com/pik/tilda/tree/vteport_old).

pik commented 9 years ago

I fixed mast distcheck breaking. There are 4 config options that will become deprecated with this, so it would be good to a big a nicer behaviour than retain them and ignore.

lanoxx commented 9 years ago

It looks like make clean does not remove src/tilda.ui, can you confirm that?

pik commented 9 years ago

It looks like make clean does not remove src/tilda.ui, can you confirm that?

Yes, I fixed that and rebased.

I think I remembered I used VTE_290 so that I wouldn't have to include in every single file just to get access to the VTE_MINOR/MICRO_VERSION conditionals - also those use vte-version (0.40/0.36) rather than lib-vte version(2.90/2.91) But I think it still may be more consistent to use their definitions, what do you think?

lanoxx commented 9 years ago

I think there is one more problem with the build system. It will always output a warning that tilda.ui could not be found even though it compiles and builds just fine. The cause seems to be this line:

$(shell $(GLIB_COMPILE_RESOURCES) --generate-dependencies $(srcdir)/src/glade-resources.gresource.xml --sourcedir=$(srcdir)/src)

And the reason is that $(shell ) is evaluated by make before the tilda.ui file has been generated.

I think we should remove this part all together and just list src/tilda.ui as a dependency directly.

A bug report already exists for this kind of problem: https://bugzilla.gnome.org/show_bug.cgi?id=673101

I have one more question, why is it necessary to use a PHONY target and always rebuild tilda.ui? Isn't there another way that allows us to just generate tilda.ui when tilda.ui.pre has changed?

Besides that you patch looks quite good, if we can fix the remaining issues with the build then I will be happy to merge this soon. I also like your approach with the deprecated config values. :+1:

lanoxx commented 9 years ago

One more thing, we should prefix the lines with cat and sed with $(AM_V_GEN) so that make will print a line like this:

GEN      src/tilda.ui

I will also add that to the gresource generated files.

lanoxx commented 9 years ago

Just noticed one more thing, in EXTRA_DIST you are distributing tilda.ui, but we probably want to distribute tilda.ui.pre?

Justinzobel commented 9 years ago

If this has been patched to support 2.91, is there going to be a new releaes ie 1.2.5 or 2.0?

lanoxx commented 9 years ago

No, this will not make it into the stable releases. I will probably make a 2.1 release once this was merged. I hope to do that in a few weeks depending on how I have time.

Justinzobel commented 9 years ago

Thanks Sebastian, looking forward to it. Will setup a notification so I see it when it hits your releases page.

On Mon, 10 Aug 2015 at 18:33 Sebastian Geiger notifications@github.com wrote:

No, this will not make it into the stable releases. I will probably make a 2.1 release once this was merged. I hope to do that in a few weeks depending on how I have time.

— Reply to this email directly or view it on GitHub https://github.com/lanoxx/tilda/pull/138#issuecomment-129373412.

Justinzobel commented 9 years ago

Hey @lanoxx just wondering how the progress is going on 2.1?

lanoxx commented 9 years ago

I finally took some time to merge this today.

I have modified the original patch a little and removed the GtkBuilder tilda.ui file generation from the tilda.ui.pre file, since I could not get this to work reliably. In particular it kept breaking the dependency generation of glib-compile-resources and caused problems when running make clean and switching branches.

Now we simply keep one ui file but hide the deprecated UI elements if we are building with VTE 2.91.

Please all test this and report back if you experience any errors.

Many thanks go to Alex (@pik) for writing most of the patch.

Justinzobel commented 9 years ago

Awesome @lanoxx any idea on an ETA for 1.3?

lanoxx commented 9 years ago

I will probably do it soon. If anyone can could test my wip/animation branch that would be nice. I would like to merge that into master before I release 1.3