Prevent false-positive when matching dark themes

[WhyNotHugo]

References: #17061

[CLAassistant]

All committers have signed the CLA.

[WhyNotHugo]

Note: I signed a DCO, not a CLA as the bot above indicates.

[ilya-fedin]

This should be in sync with the xesttings part

[WhyNotHugo]

Updated.

[ilya-fedin]

Maybe it's better to create a helper function in private namespace? IsGtkDarkTheme or something like that.

[WhyNotHugo]

Yes, will do.

[ilya-fedin]

bool IsGtkDarkTheme(const QString &theme) {
	const auto themeLower = theme.toLower();
	return themeLower.contains(qsl("-dark-")) || themeLower.endsWith(qsl("-dark"));
}

[WhyNotHugo]

in private namespace

I'm not very familiar with C++ and I'm not sure about this bit.

[23rd]

You can squash all commits to the first one. 👌

[WhyNotHugo]

Squashed!

[WhyNotHugo]

I'm curious, why this:

			if (IsGtkDarkTheme(gtkThemePortalString)) {
				result = true;
				return result;
			}

Instead of:

			if (IsGtkDarkTheme(gtkThemePortalString)) {
				return true;
			}

Also, result = false repeats a lot, but it's not necessary since it only ever changes to true before returning. Would this make more sense?

-	std::optional<bool> result;
+	std::optional<bool> result = false;

[ilya-fedin]

I'm curious, why this

We're checking all the APIs and return as soon as at least one of them returns dark mode

Also, result = false repeats a lot, but it's not necessary since it only ever changes to true before returning. Would this make more sense?

The default value of result is std::nullopt (what means 'not supported'), not false (what means light mode)

[john-preston]

@WhyNotHugo what's DCO?

[john-preston]

@WhyNotHugo I see, Developer Certificate of Origin.

The point of the CLA is to give the submitted work to the public domain so that if I need to do any relicensing in the future I won't need to contact all the people who sent PRs here.

[WhyNotHugo]

Hmm, upon further inspection, there's a lot of inconsistency here:

• The bot claims that one has to sign a CLA.
• The agreement shown has a title "Developer Certificate of Origin".
• The actual body of the agreement is not the DCO, but something very different that releases the contribution into public domain.

I had missed the discrepancy between title and body the previous time. Isn't it misleading to call it a DCO if the text is entirely different and has different goals?

I'm also a bit confused about the implications here: Telegram is distributed under the GPL, but if all developers have signed this agreement, doesn't that imply that it's fully been released into public domain?

The point of the CLA is to give the submitted work to the public domain so that if I need to do any relicensing in the future I won't need to contact all the people who sent PRs here.

I'm familiar. Using a CLA in this style is quite common for open source projects that later want to turn into a non-open source or non-free project.

[john-preston]

@WhyNotHugo Yes, for example I've maintained a closed source version for Telegram Support volunteers, that was later merged into the current open source version. But still I leave a possibility for something like that to happen again. Sorry, it is what it is.

There are no plans to close the source code of any Telegram app (all the apps and the client side libraries for building other apps, like TDLib, are open source and this is not planned to be changed). But still I need the full control of the source code, sorry.

[john-preston]

Perhaps the phrasing could be improved indeed.

[ilya-fedin]

@WhyNotHugo if you don't agree with the CLA, maybe it's better to close the PR?

[WhyNotHugo]

Oh, pretty sure I did accept it, I'll do that again.

[WhyNotHugo]

Perhaps the phrasing could be improved indeed.

Including the document in the repo too would probably be good enough, making it a lot less confusing/ambiguous what people agreed to. Especially since the ACTUAL body of the agreement is hosted externally and all references made by the bot are are not quite accurate.

[WhyNotHugo]

Oh, can't sign right now since bot is down anyway. Will retry later.

[WhyNotHugo]

Looks like the CLA bot now needs a few more permissions that are, regrettably, not at all reasonable (e.g.: read-write access to several private and public resources):

[ilya-fedin]

So, will you sign?

[vimpostor]

Looks like the CLA bot now needs a few more permissions that are, regrettably, not at all reasonable (e.g.: read-write access to several private and public resources):

Maybe try something similar to this: cla-assistant/cla-assistant#243 (comment)
CLA Assistant only needs those permissions if you setup a CLA yourself, these permissions shouldn't come up when you just want to sign one.

[WhyNotHugo]

@vimpostor Thanks for the hint. It looks like https://cla-assistant.io/telegramdesktop/tdesktop should work.

I though the CLA Assitant was down again:

Apparently not, it loads the body via an XHR call which takes 26 seconds to load:

Given that the page loads in under 2 seconds, it never occurred to me that there was an artificial delay of 26 seconds before someone could read the agreement. There's no clue that one has to wait (and I'm curious why that wait is enforced).

For the record, the CLA currently reads:

Telegram Desktop Developer Certificate of Origin

By making a contribution to this project, I certify that:

(a) The contribution was created in whole by me or is based upon previous work that, to the best of my knowledge, is in the public domain and I have the right to put it in the public domain.

(d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all metadata and personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed.

(e) I am granting this work into the public domain.

I do still believe that having the document in-repo would be a simpler and more transparent approach.

[vimpostor]

I do still believe that having the document in-repo would be a simpler and more transparent approach.

I agree but I guess the advantage of the current approach is that Preston can see directly in the PR if someone signed the CLA.

If you put the CLA just in the repo, you would also need to require every contributor to signoff their commit (similar to the Linux Kernel) with Signed-off-by: and you still wouldn't be completely sure that they agreed to the CLA. Also I am not sure if you can apply the DCO Signoff workflow at all to a CLA and still be legally sound.

[WhyNotHugo]

Given 17fcc72, this is no longer relevant. Thanks @ilya-fedin for implementing that! 🍻