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

Transparency Support And Config File Changes. #84

Merged
merged 7 commits into from Apr 5, 2021
Merged

Transparency Support And Config File Changes. #84

merged 7 commits into from Apr 5, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jan 1, 2021

No description provided.

@ghost
Copy link
Author

ghost commented Jan 1, 2021

Only tested on linux.

@peterbrittain
Copy link

Been trying to catch up on the various threads about transparency here... I think this is the latest, so posting my comments here. Apologies if this is the wrong thread.

Two immediate questions/comments about the use of -1 to access default colours:

  1. How widespread is that supported? Asciimatics uses the setaf/setab codes to change colour and I couldn't find any obvious docs that show -1 is an accepted standard for selecting fg/bg default colours with these escape sequences.
  2. This will not work in Windows. At a minimum, it needs to map -1 to a valid colour before calling into asciimatics (or adding support for default colours to asciimatics instead).

@ghost
Copy link
Author

ghost commented Jan 3, 2021

@peterbrittain i really don't know much about terminals i am fairly new to the console world but i have requested for this same feature previously in various other console applications and all of the discussions ended up with the same "-1" implementation.I know this works on linux as that is the only operating system i use.

Maybe this could be merged as a linux only feature and if not I'll be happy to make a fork.

The PR is fully tested and working on a linux system. I can upload screenshots if necessary.

@pawamoy
Copy link
Owner

pawamoy commented Jan 3, 2021

Hello @blackCauldron7, thank you very much for the PR!

I'll see how it behaves on a Windows machine before merging. I think we can also use DEFAULT by default in the config file instead of adding a commented block.

@peterbrittain
Copy link

@blackCauldron7 When you say you know it works on Linux, I think this is undefined behaviour and so will vary for each terminal implementation... Which terminals has this been tested this on? Or can one of your previous requests shed more light on why this is safe?

@ghost
Copy link
Author

ghost commented Jan 5, 2021

@peterbrittain i can confirm it works on kitty, alacritty and st.I can understand the concerns about this method but maybe release it as an experimental version and wait and see if issues arise, ill be happy to look at the issues and if they are unfixable then remove the experimental feature.

And exactly because of those concerns i provided a commented block in the config file rather than making this the default.

@peterbrittain
Copy link

I had a quick play with gnome terminal on the asciimatics demos and... It doesn't work. :-(

This basically comes back to the trail from gitter... The only way to make this genuinely work for all systems is to use the right ansi escape code (https://en.wikipedia.org/wiki/ANSI_escape_code) to select the default background - i.e. SGR 49 instead of SGR 48 with an invalid (negative) index. It's possible to provide logic to do this (looking up the "op" terminfo capability) and then to provide a hard-coded mapping in Windows.

@pawamoy
Copy link
Owner

pawamoy commented Feb 17, 2021

Hey @peterbrittain, sorry for answering so late, and thank you for helping here, I appreciate it very much 🙂
So, if I understand correctly, you would be inclined to providing such logic in asciimatics?

@blackCauldron7 sorry as well for answering so late! I guess you were right and the option, if we merge it, should be optional and not the default. I still want to run some tests before deciding. Maybe we could also see if something can be done in asciimatics, see previous comment. Anyway thanks again for the PR! I'll try to answer more quickly next time!

@peterbrittain
Copy link

Yeah - I need to think about it a little, but reckon we can do it.

@ghost
Copy link
Author

ghost commented Feb 18, 2021

Lemme know if i can be of any help.

@peterbrittain
Copy link

I think I've just pushed the necessary changes. Will need to check on a Windows system, but should work for Linux. If you want to try, changes are now available on master.

@ghost
Copy link
Author

ghost commented Mar 6, 2021

Should i close the PR?

@peterbrittain
Copy link

I don't think so... aria2p now needs to pass Screen.COLOUR_DEFAULT to asciimatics when you want those colours, so you'll still need a change here.

@peterbrittain
Copy link

Let me know 8f you need a new release to get this working.

@ghost
Copy link
Author

ghost commented Mar 7, 2021

Can you elaborate what I really have to do is there a change from ascimattics library?

@peterbrittain
Copy link

Sure. As of yesterday, asciimatics supports a new colour Screen.COLOUR_DEFAULT. You will need to use that when you want the default foreground or background. I think you just need to use that instead of -1 in yout current pull request.

@ghost
Copy link
Author

ghost commented Mar 7, 2021

Wait are you the maintainer of ascimattics?

@peterbrittain
Copy link

Guilty as charged!

@pawamoy
Copy link
Owner

pawamoy commented Apr 2, 2021

Hey @blackCauldron7, @peterbrittain, I was able to test this on both Windows and Linux, with the new Screen.COLOUR_DEFAULT, and it works fine 🙂

I think this time we can set the default to COLOUR_DEFAULT in the code.

@blackCauldron7 do you want to send these changes? Otherwise I can do it myself 🙂

@ghost
Copy link
Author

ghost commented Apr 4, 2021

@pawamoy sure go for it i guess you already have a working build. Should i close this PR?

@pawamoy
Copy link
Owner

pawamoy commented Apr 4, 2021

No no leave it open! I can add commits to it directly. You'll be credited in the git history and changelog :)

@pawamoy
Copy link
Owner

pawamoy commented Apr 4, 2021

@peterbrittain do you plan on releasing a new version of asciimatics soon?

@peterbrittain
Copy link

I can create a new release if that helps. Just let me know...

@pawamoy
Copy link
Owner

pawamoy commented Apr 5, 2021

Yes please 😄 This way we'll be able to specify the new version in our pyproject.toml 🙂

@peterbrittain
Copy link

Just tried... Picked the wrong day to do it, though! Pypi is down due to a certificate problem.

@pawamoy
Copy link
Owner

pawamoy commented Apr 5, 2021

Ah, no problem, another day then 🙂

@peterbrittain
Copy link

Looks like they're back up and running... 1.13.0 is just going live now!

@pawamoy pawamoy merged commit ff35d2b into pawamoy:master Apr 5, 2021
@pawamoy
Copy link
Owner

pawamoy commented Apr 5, 2021

Just squashed and merged, you have nothing else to do 🙂
Thank you very much again for your contribution @blackCauldron7!
And thanks @peterbrittain for your help and asciimatics release!

@peterbrittain
Copy link

You're welcome!

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

2 participants