Fix editing items in tree making changes to the wrong item

[EricEzaM]

This was caused by the fact that selecting a new item would immediately change that item to the "popup_edited_item" which meant that when the item_edited method was called, the wrong item was being changed. "popup_edited_item" would change immediately upon selection because bring_up_editor was based on it's current selection status, which could have changed to true just before, in the same method, where selection is handled (see line 1758). This change makes it based upon it's previous selection status (which was already used) so that the editor does not popup immediately on first selection.

This was a pretty big pain to troubleshoot tbh. Tree could do with a refactor... It is quite messy and hard to decipher.

Closes #42419

[akien-mga]

Bug #42419 sounds like a regression (otherwise I doubt it would have gone unnoticed so long if it's in 3.x too), so it would be good to figure out what caused the regression before attempting a fix.

This line hasn't been modified since 2017 (cbcf40b), so if there's a regression it's triggered by another part of the code.

[EricEzaM]

Yeah it is a regression and only occurs in 4.0, cant repro in 3.X.

I had a look at the commit history of tree.cpp and couldn't see anything that would affect it... Bit I guess bisecting would be the best option.

[EricEzaM]

After a long night of bisecting... it appears b3080bc is the first bad commit.

Confirmed after testing it and the previous commit, 09ba290, which does not exhibit the issue. I'll have to look into this tomorrow.

[akien-mga]

Ouch, sorry I sent you on a chase after such an old mid-refactoring commit. I thought the regression might have been newer than that.

[EricEzaM]

All good, I'm generally on a goose chase of a few bugs and fixes while working on #42351 and godotengine/godot-proposals#1532

All part of the fun haha :)

[EricEzaM]

@akien-mga After a quick review, I think this still might be the correct solution.

See in the below gif, which is before this fix, when you click on another item it starts editing it immediately? I dont think this is the correct behaviour. It is also what causes the bug attached to this PR.

When you click on an item it should stop editing the previous item but not start editing the one you clicked on:

It's like on Windows if you are renaming a file, if you then click on another file it does not start immediately editing the one you clicked on.

Additionally the behaviour which occurs before this fix means that if, in response to the item_edited signal, you call tree->get_edited(), you will get the item you clicked on, as opposed to the one you actually edited.

[akien-mga]

If you can confirm that this should still be the correct fix, we can likely merge as is @EricEzaM, unless someone else in @godotengine/gui-nodes has a strong opinion about this.

[Paulb23]

This seems to be already working as intended, tested this in Godot v3.3.2 and it works the same as 4.0.

The editor should only appear on first click when allow_reselect is false, setting it to true produces the behaviour seen here.

Might be worth changing the default value from false to true?

[KoBeWi]

Might be worth changing the default value from false to true?

Wouldn't it basically mean that using non-default value would result in broken behavior? Changing default isn't enough to fix the issue.

[Paulb23]

Wouldn't it basically mean that using non-default value would result in broken behavior?

As far as I know it is working as intended, with this PR allow_reselect no longer does anything. If the users expectations is what we see here, changing the default makes the current behaviour opt-in which should reduce confusion.

[KoBeWi]

How is the behavior from the GIFs intended? Selecting an item will overwrite its value with the value of the previously selected one. This is definitely a bug.

Still, if the fix breaks something else, it's not correct.

[Paulb23]

Ahh, no idea how I missed that, yeah that's not indented.

[EricEzaM]

Can't repro issue any more