Replace Tree control left click on title signal with new signal for either left or right click

[derammo]

Right mouse button support for clicking on the "column title" headers in the Tree control was apparently missed when right click support was added for other items.

This interaction style is needed to support a context menu to configure hiding/showing of optional columns in a normal way.

@KoBeWi as discussed on chat.

[derammo]

@YuriSizov does 4.x mean deferred past 4.0 (i.e. not anytime soon?) Because I need this for current development of the debugger. [edit: Not "need" as much as "I am using it." I guess I can go rework the UI?]

[YuriSizov]

No, 4.x means "any 4.x version". If this is reviewed and approved, it can be merged in 4.0. But as it stands, it doesn't seem essential for the 4.0 release specifically.

The debugger work doesn't seem to be approved yet by any maintainer, so I don't know if we want it in 4.0 or if it would require breaking compatibility. But if #64364 is approved for 4.0, then this can be moved as well.

[derammo]

Thank you for the detailed explanation. That sounds perfect.

[derammo]

And just for clarity: this adds a new signal and intentionally does not remove the previous version of the signal, so that it does not break compatibility. The legacy signal continues to be fired in the same cases as before.

[kleonc]

And just for clarity: this adds a new signal and intentionally does not remove the previous version of the signal, so that it does not break compatibility. The legacy signal continues to be fired in the same cases as before.

If such new signal is gonna be added then it makes sense to do break compatibility and remove the old signal as it would be redundant. Such change of course would need to be introduced in 4.0 (because of breaking compatibility).

If you'd want to backport it to 3.x then yes, the old signal in there would need to be not removed.

I suggest/prefer removing the old signal in this PR (just my opinion).

[derammo]

And just for clarity: this adds a new signal and intentionally does not remove the previous version of the signal, so that it does not break compatibility. The legacy signal continues to be fired in the same cases as before.

If such new signal is gonna be added then it makes sense to do break compatibility and remove the old signal as it would be redundant. Such change of course would need to be introduced in 4.0 (because of breaking compatibility).

If you'd want to backport it to 3.x then yes, the old signal in there would need to be not removed.

I suggest/prefer removing the old signal in this PR (just my opinion).

I don't think I can accept responsibility for hunting down all the code that uses the old signal and fixing it, especially at a time when things are supposed to stabilize for 4.0? I don't know how to proceed.

[kleonc]

I don't think I can accept responsibility for hunting down all the code that uses the old signal and fixing it, especially at a time when things are supposed to stabilize for 4.0?

Responsibility is also on the reviewers/approvers/merger, that's how I see it. And searching for the signal name in the whole codebase and going through all the findings doesn't look too hard / error prone. Especially since I've justed searched for that old signal (column_title_pressed) and seems like it's not used in the whole codebase (in occures only in the Tree itself and its docs). So removal seems trivial.

I don't know how to proceed.

As I pointed out, it was just my opinion. Whether others will agree with me that it should go into 4.0 I don't know. So I guess you can just wait for other opinions / decision. If you want you could add a second commit removing the old signal to show what it's about (and later based on the decision you would either remove that commit or squash both commits into one).

[derammo]

Responsibility is also on the reviewers/approvers/merger, that's how I see it. And searching for the signal name in the whole codebase and going through all the findings doesn't look too hard / error prone. Especially since I've justed searched for that old signal (column_title_pressed) and seems like it's not used in the whole codebase (in occures only in the Tree itself and its docs). So removal seems trivial.

But Tree is a public API class

GDREGISTER_CLASS(Tree)

I thought that means people can be using it in their Games' UIs or Editor Plugins and we'd never know?

As I pointed out, it was just my opinion. Whether others will agree with me that it should go into 4.0 I don't know. So I guess you can just wait for other opinions / decision. If you want you could add a second commit removing the old signal to show what it's about (and later based on the decision you would either remove that commit or squash both commits into one).

I don't have enough experience with the Godot dev process I guess. I will just wait a while and see what other comments happen. Maybe it is normal to just remove API or maybe I misunderstood what is public API. I do appreciate your careful review!

[kleonc]

But Tree is a public API class
GDREGISTER_CLASS(Tree)
I thought that means people can be using it in their Games' UIs or Editor Plugins and we'd never know?

Yes and yes. But Godot 4.0 is not meant to be backwards compatibile with 3.x, there are already many compatibility breaking changes between these versions. And, since 4.0 is still in the alpha stage, new breaking changes can still be introduced. I think it's pointed out in each 4.0.alpha release note that the things can change (be removed/changed/added and so on). So it's not like the users are not aware of this. E.g. from the latest release note:

Be aware that during the alpha stage the engine is still not feature-complete or stable. There will likely be breaking changes between this release and the first beta release. Only the beta will mark the so-called "feature freeze".

As such, we do not recommend porting existing projects to this and other upcoming alpha releases unless you are prepared to do it again to fix future incompatibilities. However, if you can port some existing projects and demos to the new version, that may provide a lot of useful information about critical issues still left to fix.

Anyway, feels like we're getting offtopic. I think I've already stated my point (I like the new signal and I think it should be a replacement for the old one, not an addition to it) and I have nothing more to say here for now.

[derammo]

Removed the previous signal as requested. This PR will now need the breaks_compat label.

[kleonc]

Removed the previous signal as requested. This PR will now need the breaks_compat label.

@YuriSizov Shouldn't the milestone be changed to 4.0 as well? I think it should be resolved before the beta.

[YuriSizov]

Shouldn't the milestone be changed to 4.0 as well? I think it should be resolved before the beta.

Sure, we can switch the milestone back and forth 🙃 But it still needs a review more than anything else, then we can adjust the metadata, once we agree if we want this or not.

[kleonc]

Code changes look fine. Signal rename and added additional parameter seem like an obvious improvement to me.

[YuriSizov]

Thanks!