Rewrite picom-trans

[subnut]

I have re-written picom-trans to be better understandable. Also added comments.

[codecov]

Codecov Report

Merging #634 (f35efd6) into next (be24c0d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             next     #634   +/-   ##
=======================================
  Coverage   39.90%   39.90%           
=======================================
  Files          46       46           
  Lines        9484     9484           
=======================================
  Hits         3785     3785           
  Misses       5699     5699           

[yshui]

Thanks for your work! I am not very experienced in bash however, @tryone144 what about you?

It feels bad to ask this after you have done all this work, but how do you feel about rewriting this script in a different language, say Python? I think things like argument parsing is going to be much simpler.

[subnut]

Actually, I had previously thought of writing it in a different language. I know python, C, and sh.

Writing it in Python would mean adding yet another dependency. The lesser dependencies, the better, IMO.

Writing it in C would not add dependency, but then, it would make a compiled binary, which will be harder to debug than an interpreted language.

Ultimately, I decided to write in POSIX sh. /bin/sh is guaranteed to exist, and the end user can easily see what commands are being executed, and even debug small problems themselves.

[subnut]

EDIT: After some googling, I have found out that yes, that use-case is actually required by GNU. (not POSIX. POSIX doesn't support long options like --help either.)

---

picom/bin/picom-trans

Lines 98 to 100 in be24c0d

	name=* | window=* | opacity=*)
	v=$(echo "$OPTARG" | sed -E 's/^[^=]+=//')
	;;

@yshui The purpose of the above lines is to allow arguments to be specified like this -

picom-trans --name=feh --opacity=80

Which shall be the equivalent of -

picom-trans --name feh --opacity 80

My question was, running picom-trans -h only shows the second use-case (--opacity 80) and not the first use-case (--opacity=80). So, do I need to implement the first use-case?

[yshui]

Writing it in Python would mean adding yet another dependency.

Python is pretty ubiquitous, it's not something I would worry about.

[subnut]

Actually, I did not re-write the whole script. I just re-structured it, re-wrote the argument-parsing from scratch, and make the whole script as POSIX-compatible as I could. That's why I stuck with sh.

Re-writing it in python would need 10 times more effort than what I did here. Also, I don't think python is really suited of this kind of text-processing task. I don't know any grep sed equivalent for python.

Also, IMO bash is even more ubiquitous than python.

[subnut]

@yshui Please tell me if you can find any bugs.

I have fixed all bugs I could find.

[yshui]

Thanks, I am still reviewing it.

[tryone144]

The restructured code is definitively more readable than the old.
This has however introduced some (breaking) changes and/or bugs. Particularly, the parsing of +-OPACITY% is still not quite working correctly.

[ghost]

printf instead of echo would be nice.

Tl;DR: At a very high level, printf is like echo but more formatting can be done.

Reasons

• Formatting: printf has more control over the output format compared to echo. It can be used to specify the field width to use for item, as well as various formatting choices for numbers (such as what output base to use, whether to print an exponent, whether to print a sign, and how many digits to print after the decimal point). This is done by supplying the format string, that controls how and where to print the other arguments and has the same syntax as C language (%03d, %e, %+d,...). This means that you must escape the percent symbol when not talking about format (%%).
• Status: echo always exits with a 0 status, and simply prints arguments followed by an end of line character on the standard output, while printf allows for definition of a formatting string and gives a non-zero exit status code upon failure.

Performance

Concerning performance, I always had in mind that echo was faster than printf because the later has to read the string and format it. But testing them with time (also built-in) the results say otherwise:

$ time for i in {1..999999}; do echo "$i" >/dev/null; done
real    0m10.191s
user    0m4.940s
sys 0m1.710s

$ time for i in {1..999999}; do printf "$i" >/dev/null; done
real    0m9.073s
user    0m4.690s
sys 0m1.310s

Telling printf to add newline characters, just as echo does by default:

$ time for i in {1..999999}; do printf "$i\n" >/dev/null; done
real    0m8.940s
user    0m4.230s
sys 0m1.460s

That is obviously slower than without printing the \n, but yet faster than echo.

[tryone144]

Thanks @subnut.
If I am not mistaken we should have reached a state that is working with all functionality from before. Parsing of the arguments should be more robust now and if anything the script appears to be a tad faster or at least not slower than before. 👍

I haven't been able to test every (im-)possible argument combination in the old implementation, but the examples at the top still give the same results.

Please take a look at the last bug I could find. Other than that, I think it is best to squash at least the bugfix / fixup commits into the commit they fix.

[subnut]

Good idea!

I will squash the bugfix commits, and also change the commit message style from [picom-trans] ... to picom-trans: ...

[subnut]

I have reduced the number of commits from 30 to 20. Do let me know if it needs to be decreased further.

I have merged all the bugfix commits with the commits which introduced the bug. Also, I have merged some other-than-bugfix commits, like, say a commit introduces a block of code that is later removed again a few commits later.

[yshui]

I am inclined to just squash the whole PR

[subnut]

@yshui

I am inclined to just squash the whole PR

But that would remove the (valuable, IMHO) commit messages, right?

[subnut]

@yshui @tryone144

Current picom-trans prints the whole path to the file when it prints error messages or help output -

$  picom-trans --help
/usr/bin/picom-trans: illegal option help

But in this PR, I've changed it to show only the basename of the file path. i.e. it will only show the last part of the whole path
e.g. the basename of /usr/bin/picom-trans is picom-trans -

$  picom-trans --helpl
picom-trans: illegal option --helpl

Is this change accepted? Or should I revert it to the old behavior of printing the whole path?

[yshui]

But that would remove the (valuable, IMHO) commit messages, right?

If there are design decisions, explanation of how code works, etc. in the commit messages, then they can also be recorded as code comments. And IMO they are more visible there.

Bug fixes (unless they are recurring pitfalls, in that case they could be code comments too), indentation fixes, etc. should be squashed away. Am I missing some other types of valuable commit messages that you had in mind?

[subnut]

OK. Agreed.

And it seems that in the midst of squashing all the commits, some important bugfixes got missed out. So I'm going to have to revert from the reflog anyway. Sigh.

Let me go find the last valid commit from the git reflog. Then I will add the necessary comments from the commit messages as comments to the code. Then we can simply squash-merge this PR.

---

@yshui What's your opinion on this change?

[subnut]

I think this PR is now ready to Squash and Merge

[tryone144]

LGTM. Thanks for your work on this @subnut 👍

[subnut]

I just now remembered, I forgot about the LICENSE!

I would like to LICENSE my code under MIT, but then again, I don't know under what license Christopher Jeffrey released the initial version upon which my work is based, and whether that license is compatible with MIT.

Hey @chjj, is it you who wrote the original picom-trans?
If yes, then what License did you release it under? MIT?

EDIT: According to this, it seems Christopher licensed his original version with the MIT/Expat License, which is the exact same license that I was intending to license my code under.

@yshui Since you are the maintainer, give me advice on where to mention about the License. Should I place it as a comment block on the top of the file? Or is there some other file?

[yshui]

@subnut put a # SPDX-License-Identifier: MIT at the top of the file would be enough.