3.5.1 - Simplify a complicated bool

[Tuxino88]

log error

../src/Utils/ThreadUtils.vala:40.13-40.95: warning: unhandled error `GLib.ThreadError' 40 | var tp = new ThreadPool.with_owned_data (worker => worker.run (), 1, false); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../src/Views/NoteContentView.vala:250.40-250.157: error: Equality operation: `bool' and `string' are incompatible 250 | back2_button.visible = ((MainWindow)MiscUtils.find_ancestor_of_type(this)).sgrid.get_visible_child_name () == "notegrid" != false ? true : false; | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vala 0.56.4
glib2 2.76.1

[musicinmybrain]

This can be simplified so that it is easier to read and does not suffer from an unintended order of operations.

back2_button.visible = (...).sgrid.get_visible_child_name () == "notegrid" != false ? true : false;

back2_button.visible = (...).sgrid.get_visible_child_name () == "notegrid" != false;

back2_button.visible = (...).sgrid.get_visible_child_name () != "notegrid";  # WRONG!

back2_button.visible = (...).sgrid.get_visible_child_name () == "notegrid";  # CORRECTED

I believe that is what was intended.

[musicinmybrain]

Based on Fedora’s Koschei tool, this broke when vala was upgraded from 0.56.3 to 0.56.4.

I was working against an old checkout. It looks like the code in question is no longer present in main. I have therefore closed #396. Here is the proposed patch for 3.5.1, in case it proves useful:

0001-Simplify-an-overcomplicated-Boolean-expression.patch

EDIT: See the discussion below for a corrected patch.

[lainsce]

Seems that the patch works, so I'll leave this pinned but closed.

[xiota]

I just encountered that patch elsewhere. The substitution looks wrong to me.

The expression looks like X==Y != false : true ? false;. The patch changes it to X!=Y. But, if X=Y:

X==Y != false : true ? false;
true != false : true ? false;
true : true ? false;
true

If X!=Y:

X==Y != false : true ? false;
false != false : true ? false;
false : true ? false; 
false

So the proper substitution should be X==Y.

Also, I see a lot of X==Y : true ? false, X!=Y : false ? true, and similar constructions. Why not use X==Y and X!=Y directly?

[musicinmybrain]

I just encountered that patch elsewhere. The substitution looks wrong to me.

[…]

So the proper substitution should be X==Y.

Yes, I think you’re right. I’m surprised I botched that simplification.

[musicinmybrain]

Yes, I think you’re right. I’m surprised I botched that simplification.

Actually, wait: if your interpretation were correct, it wouldn’t be complaining about comparing a string to a boolean. That must be the “unintended order of operations” I was talking about. Maybe equality comparisons in Vala are right-associative rather than left-associative? It’s been a while; I’ll try to revisit this more thoroughly, but not tonight.

[xiota]

Maybe equality comparisons in Vala are right-associative rather than left-associative?

That would be interesting. This is why parentheses were invented.

[musicinmybrain]

Rather than choose to untangle the nuances of Vala syntax and how it’s apparently changed over time, I decided to just try an experiment:

void main () {
    bool value = "notegrid" == "notegrid" != false ? true : false;
    print (value ? "true\n" : "false\n");
}

demo.vala:2.18-2.50: error: Equality operation: `bool' and `string' are incompatible
    2 |     bool value = "notegrid" == "notegrid" != false ? true : false;
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                

Trying again with an old version of valac, 0.40.19:

true

Changing the first "notegrid" to "potato":

false

So before something changed in Vala to break the original code, it behaved as X==Y in practice, just as you suggested (and as a careful simplification by C syntax rules would predict).

---

Next, I built Notejot 3.5.1 with the corrected patch and tested it interactively:

• Click “Note Grid”
• Click ”+” to create a new note (if there isn’t one already)
• Click the note
• Observe that a “<” button appears at the top left of the right panel to return to the grid view

If I repeat this with the incorrect patch, the back button does not appear in the last step.

So I do still agree with you: the correct patch should be:

From c6a7cfcb792de63fb51eb174f9f3d4e02f6a2ce1 Mon Sep 17 00:00:00 2001
From: "Benjamin A. Beasley" <[email protected]>
Date: Fri, 14 Apr 2023 19:35:47 -0400
Subject: [PATCH] Simplify an overcomplicated Boolean expression

Fixes an order-of operations issue (fixes #395).
---
 src/Views/NoteContentView.vala | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/Views/NoteContentView.vala b/src/Views/NoteContentView.vala
index ba6c608..702faae 100644
--- a/src/Views/NoteContentView.vala
+++ b/src/Views/NoteContentView.vala
@@ -247,7 +247,7 @@ public class Notejot.NoteContentView : View {
             if (((Adw.Leaflet)MiscUtils.find_ancestor_of_type<Adw.Leaflet>(this)).folded) {
                 back2_button.visible = false;
             } else {
-                back2_button.visible = ((MainWindow)MiscUtils.find_ancestor_of_type<MainWindow>(this)).sgrid.get_visible_child_name () == "notegrid" != false ? true : false;
+                back2_button.visible = ((MainWindow)MiscUtils.find_ancestor_of_type<MainWindow>(this)).sgrid.get_visible_child_name () == "notegrid";
             }
             back2_button.clicked.connect (() => {
                 ((Adw.Leaflet)MiscUtils.find_ancestor_of_type<Adw.Leaflet>(this)).set_visible_child (((MainWindow)MiscUtils.find_ancestor_of_type<MainWindow>(this)).sgrid);
-- 
2.41.0

Thanks for pointing out the error.

Hopefully a new release will come along soon and make all of this moot.

[xiota]

@musicinmybrain Thanks for taking the time to double check and revise the patch. The experiments to confirm vala behavior was a good idea.

Would you happen to have any idea why there are so many constructions similar to X==Y ? true : false and X!=Y ? false : true? (Just realized I mixed up the : and ? in my earlier comment.)

[musicinmybrain]

Would you happen to have any idea why there are so many constructions similar to X==Y ? true : false and X!=Y ? false : true? (Just realized I mixed up the : and ? in my earlier comment.)

No, I have no idea. I’m just a drive-by contributor, and the maintainer of the notejot package in Fedora Linux. These constructs seem overcomplicated and error-prone to me.

[xiota]

These constructs seem overcomplicated and error-prone to me.

I don't know anything about vala and was wondering if they might be a vala thing... I was thinking about making a patch / pull request to clear them all out, but I'm having difficulty building this project from git (#407). Would you happen to know of a working commit subsequent to 3.5.1 (with your patch applied)?

[musicinmybrain]

These constructs seem overcomplicated and error-prone to me.

I don't know anything about vala and was wondering if they might be a vala thing... I was thinking about making a patch / pull request to clear them all out, but I'm having difficulty building this project from git (#407). Would you happen to know of a working commit subsequent to 3.5.1 (with your patch applied)?

My patch wasn’t applied. The code in question was completely refactored at some point after 3.5.1, which removed the line with the bug and made the patch obsolete.

I don’t work on notejot development, and I haven’t tried to build anything but stable releases.

If you’re working on a PR, make sure you’re aware how much main has changed from the 3.5.1 release: 3.5.1...main

[xiota]

My patch wasn’t applied.

What I meant is... even the 3.5.1 release doesn't build without your patch.

make sure you’re aware how much main has changed from the 3.5.1 release

I'm aware, but I would need a working commit to start from because it doesn't make sense to blindly open pull requests without at least checking that they build. I'm probably not going to bother anymore with this project. There are too many broken commits, and the convoluted code makes me hesitant to even run it.