WebAPI: Unify torrent category related keys

[jagadam97]

This PR introduces two new keys for maindata response namely
downloadPath and downloadPathEnabled.

in editCategory/createCategory we take these keys to set category
download_path but while returning the in maindata response we are
sending download_path and this fails for type strict application as
this same key return false or path based on if the download_path is
enabled or not.

leaving the download_path as is as there might be applications which
consume the download_path. (can remove if needed)

closes #23780

[glassez]

https://github.com/qbittorrent/qBittorrent/blob/master/CODING_GUIDELINES.md

[thalieht]

Coding style comments

[jagadam97]

@thalieht
thanks for pointing me towards code styling.

will provide better code in future contributions.

[Chocobo1]

Could you reorder and simplify it a bit?

Suggested change

	const BitTorrent::CategoryOptions categoryOptions = session->categoryOptions(categoryName);
	auto category = categoryOptions.toJSON().toVariantMap();
	QVariant downloadPathVal = category.value(u"download_path"_s);
	// adjust it to be compatible with existing WebAPI
	category[u"savePath"_s] = category.take(u"save_path"_s);
	category.insert(u"name"_s, categoryName);
	if ((downloadPathVal.typeId() == QMetaType::QString) && !downloadPathVal.toString().isEmpty())
	{
	category[u"downloadPathEnabled"_s] = true;
	category[u"downloadPath"_s] = downloadPathVal.toString();
	}
	else
	{
	category[u"downloadPathEnabled"_s] = false;
	}
	const BitTorrent::CategoryOptions categoryOptions = session->categoryOptions(categoryName);
	auto category = categoryOptions.toJSON().toVariantMap();
	// adjust it to be compatible with existing WebAPI
	category[u"savePath"_s] = category.take(u"save_path"_s);
	category.insert(u"name"_s, categoryName);
	// ADD SOME COMMENT FOR ADDING THIS CODE...
	const QString downloadPathVal = category.value(u"download_path"_s).toString();
	if (!downloadPathVal.isEmpty())
	{
	category[u"downloadPathEnabled"_s] = true;
	category[u"downloadPath"_s] = downloadPathVal;
	}
	else
	{
	category[u"downloadPathEnabled"_s] = false;
	}

[jagadam97]

my first approach was this but changed it at 1st rewrite thinking this might be one of the coding style discrepancy

[jagadam97]

Made the changes @Chocobo1

[Chocobo1]

I doubt you need this code here. What happens if you don't add them?

[jagadam97]

let me test it out and get back.

[glassez]

I doubt you need this code here.

What made you doubt it?

[glassez]

What happens if you don't add them?

It doesn't add the corresponding data in the snapshot.

[Chocobo1]

What made you doubt it?

It seems suspicious it require two instances of similar code.
Also, it is been some time I looked into this code section and I'm not familiar with it anymore.

[glassez]

It seems suspicious it require two instances of similar code.

An initial snapshot is generated in one place, and the modified data is serialized in another.
Perhaps the shared code could be extracted into a separate function for reuse.

[glassez]

@qbittorrent/web-developers
The question is, shouldn't we unify it in the opposite way, i.e. so that it matches the naming used in core CategoryOptions serialization? After all, it contains other items that were added recently, which sooner or later we will have to add to the WebAPI.
If not, then I don't see any point in using CategoryOptions::toJSON() as a basis at all, but just add a separate serialization function to the WebAPI code instead.

[glassez]

Everyone always forgets about it (or doesn't know at all), but the sync algorithm requires that the source dictionaries contain all the keys unconditionally.
So else branch must add the same keys as if.

[jagadam97]

I was having this dought but seeing the maindata api sending data seemlessly without maintaining the downloadPath in both branches i was convinced not to do the above.

made the change to explicity setting the all the keys.

[Chocobo1]

The question is, shouldn't we unify it in the opposite way, i.e. so that it matches the naming used in core CategoryOptions serialization?

I'm a bit out of touch with the details but I agree with the general idea. That is, match with the core option names.

[glassez]

I'm a bit out of touch with the details but I agree with the general idea. That is, match with the core option names.

We have two options for such data structures that need to be serialized for different purposes (for example, to save them in files and transfer them via the WebAPI):

1. We have a single shared serialization method (for example, defined in the class itself) and we use it in all cases. The advantage is that it is easier to keep it up to date when the structure is updated. The disadvantage is that changing it immediately affects the WebAPI, but it can have undesirable consequences if it is not handled appropriately there (at least it requires a WebAPI version change, but it can also lead to breaking changes).
2. We define a separate serialization method in each place where it is used. The disadvantage is that when the structure changes, it requires handling serialization in several places. But at the same time, the advantage is that each serialization method must be handled explicitly, which allows it to be done in a more appropriate way for each case, and it does not require doing it at the same time, which would allow for more flexible compatibility.

While the first option seems to be the best at first glance (which is why it was implemented initially), now I would most likely prefer the second one.

[jagadam97]

Hi Guys

@glassez @Chocobo1 @thalieht

I have updated the PR to use common function at both making the snapshot and making the maindata.

also i have kept if and else have same set of keys.

if we are planning to go with another approach, please do let me know and I can make the changes.

[glassez]

if we are planning to go with another approach, please do let me know and I can make the changes.

I would still prefer that we use some kind of strict approach to serialization. The current version looks like a workaround.
Since no one expresses any opinions about this, the only persistent opinion remains mine, namely:

now I would most likely prefer the second one

I.e.:

We define a separate serialization method in each place where it is used. The disadvantage is that when the structure changes, it requires handling serialization in several places. But at the same time, the advantage is that each serialization method must be handled explicitly, which allows it to be done in a more appropriate way for each case, and it does not require doing it at the same time, which would allow for more flexible compatibility.