Don't use integers in place of booleans in RPC

[lighterowl]

A lot of places in transgui use integers in RPC requests where the daemon expects booleans.

A recent commit in Transmission started actually enforcing this as the JSON parsing implementation has changed.

Originally reported in lighterowl#79 and cherry-picked from lighterowl@409512f.

[cr-gpt]

Based on the provided code patch, here is a brief code review:

1. The change in line 157 seems to be replacing the check for an integer value with a check for a boolean value.

2. Previously, it checked if res.Objects['arguments'].Integers['port-is-open'] is not equal to zero. Now, it checks if res.Objects['arguments'].Booleans['port-is-open'] evaluates to true.

3. The change seems fine and would work as intended based on the assumption that res.Objects['arguments'].Booleans['port-is-open'] accurately represents whether the port is open or not.

Improvement suggestions:

1. It's generally a good practice to add comments explaining the purpose of the code, especially when making changes. Consider adding a comment near this code block explaining the purpose of the condition and what it checks for.

2. Ensure that the data types returned by res.Objects['arguments'].Booleans['port-is-open'] and res.Objects['arguments'].Integers['port-is-open'] are consistent and match the expected values ('port-is-open' field should always return a boolean).

3. Consider handling any potential exceptions or error conditions that may arise during the execution of this code block and provide appropriate error messages or logging.

Overall, without additional context or seeing the surrounding code, it is challenging to provide an in-depth review. As a best practice, ensure thorough testing to verify that the code behaves as expected in various scenarios.

[what-the-diff]

PR Summary

• Simplified Logic in btTestPortClick Procedure
  Changes were made to the logic that checks if a port is open. It's now a simple truth check, rather than a not-zero check. This change makes the code easier to understand and maintain.

• Locked Start Button during Test Execution
  The Start button on the form is now disabled promptly after a testing process starts. This helps in preventing users from accidentally initiating another test process while one is already running.

• Re-enabled Start Button Post Test and After Checking Status
  The Start button is now promptly re-enabled after the testing process has completed, or when the status check procedure is called. This ensures that users can start a new test as soon as one is done or whenever the status is checked.

• Improved Error Handling Mechanism
  In case of an exception or an error, the Start button is immediately re-enabled. This allows users to start a new test after fixing errors without having to restart the application.

These enhancements aim to enforce better user interface management, limiting potential mishaps caused by accidental multiple clicks, as well as ensuring a smoother user experience during the testing process.

[ghost]

👇 Click on the image for a new way to code review

Legend

[fredo-47]

I can confirm that transgui is broken when used with newer transmission-daemon dev-versions, regarding functions "Move torrent data" and "Delete torrent data".

After I built transgui with the file changes suggested above, transgui is able to perform these functions again as expected.

[PeterDaveHello]

@fredo-47 Thanks for the contribution. I will take my spare time to review them as soon as possible.

[killemov]

A bot wrote:

It's generally a good practice to add comments explaining the purpose of the code, especially when making changes. Consider adding a comment near this code block explaining the purpose of the condition and what it checks for.

Too bad the bot does not warn against over-commenting, so much comments that important stuff gets lost or comments that state the obvious. Only if requesting the field from the request argument had some unforeseen side-effect, a comment about that would be valid otherwise the code itself is clear enough.

[killemov]

Can anyone tell me if the used JSON parser can handle truthy and falsy values correctly?

[lighterowl]

Can anyone tell me if the used JSON parser can handle truthy and falsy values correctly?

It's not that simple to determine because you need to explicitly state the type you're accessing when fetching values from a parsed object. However, when you access Booleans, it seems to kind-of-sort-of have the "truthy-falsy" behaviour. Have a look yourself :

unit TestCase1;

{$mode objfpc}{$H+}

interface

uses
  Classes, SysUtils, fpcunit, testregistry, fpjson, jsonparser;

type

  JsonTruthyFalsy= class(TTestCase)
  published
    procedure BoolAsInteger;
    procedure StringboolAsInteger;
    procedure StringboolAsBool;
    procedure StringAsBool;
    procedure IntegerAsBool;
  end;

implementation

procedure JsonTruthyFalsy.BoolAsInteger;
var
  obj : TJSONObject;
begin
  obj := GetJSON('{"foobar":true}') as TJSONObject;
  AssertEquals(obj.Integers['foobar'], 1);
end;

procedure JsonTruthyFalsy.StringboolAsInteger;
var
  obj : TJSONObject;
begin
  obj := GetJSON('{"foobar":"true"}') as TJSONObject;
  AssertEquals(obj.Integers['foobar'], 1);
end;

procedure JsonTruthyFalsy.StringboolAsBool;
var
  obj : TJSONObject;
begin
  obj := GetJSON('{"foobar":"true"}') as TJSONObject;
  AssertTrue(obj.Booleans['foobar']);
end;

procedure JsonTruthyFalsy.StringAsBool;
var
  obj : TJSONObject;
begin
  obj := GetJSON('{"foobar":"transgui"}') as TJSONObject;
  AssertTrue(obj.Booleans['foobar']);
end;

procedure JsonTruthyFalsy.IntegerAsBool;
var
  obj : TJSONObject;
begin
  obj := GetJSON('{"foobar":1}') as TJSONObject;
  AssertTrue(obj.Booleans['foobar']);
end;

initialization
  RegisterTest(JsonTruthyFalsy);
end.

Out of those, StringboolAsInteger and StringAsBool fail.

This means that "true" and 1 are converted to true when using .Booleans.

[killemov]

Good, this means the original Integer "getter" and comparison to 0 were actually bad programming and that it's fixed even if the actual values in the JSON are 0 or 1.