Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Reworked and more capable ConnectDialog #27988

Closed
wants to merge 6 commits into from

Conversation

samdze
Copy link
Contributor

@samdze samdze commented Apr 13, 2019

This ConnectDialog gives a comprehensive set of tools to connect signals and to manage their arguments, all in a very discoverable way and still being simple in its initial appearance.

Currently implemented:

  • Refactored PropertySelector to make it possible to create one inside any part of the interface. (not only in a ConfirmationDialog)
  • There are now two distinct modes in the ConnectDialog: "New Method" and "Existing Method" modes.
  • Existing Method mode lets the user select the method of choice inside a list of available methods. The list can be searched and the methods descriptions can be viewed in real-time.
  • Adjustments in the UI of the Dialog: Deferred/Oneshot checkboxes and "Edit Arguments..." button.
  • Added new bindable argument types: Object, Array, Dictionary, NodePath, PoolArray...
  • Added arguments info in the main dialog.
  • Added locked arguments info in the bindings dialog for arguments passed directly by the emitting signal.
  • Children nodes of instanced scenes are displayed in the Connect Dialog SceneTree if Editable Children is active.
  • Fixed (Connecting From) not displaying in "Existing Method" mode.
  • The script and instanced scene icons now always appear in the scene tree, with tooltips.
  • Disable "New Method" mode if Script Editor is disabled in the current Editor Features profile.
  • Added warnings for "inexistent method" in "Existing Method" mode and for "method already defined" in "New Method" mode.
  • Auto binding of the right amount/types of arguments when selecting an existing method.
  • Bind arguments with proper names displayed so that their intent/purpose is clearer.
  • More...

(Outdated gif)
fkVtIHL

(Newer gif)
Newer gif

@groud groud added this to the 3.2 milestone Apr 13, 2019
@groud
Copy link
Member

groud commented Apr 13, 2019

Looks great ! :)

@fede-Raider
Copy link

Great and useful 😁👍

@neikeq
Copy link
Contributor

neikeq commented Apr 13, 2019

I thought @reduz was working on this right now.

@ghost
Copy link

ghost commented Apr 13, 2019

It would be great if the source node's text in the "Existing Method" view was made to match with its appearance in the "New Method" view: colored blue with "(Connecting From)" appended. I think reduz forgot to change it.

@reduz
Copy link
Member

reduz commented Apr 20, 2019

This is too complex imo, and beats the purpose of the work I did. I was thinking something much simpler, like this (horrible mock-up).

image

Just a single button that shows a pop-up with the available script functions in that node that match the function signature of what you are trying to connect at, and that's it. The rationale is that this dialog needs to be as simple ans straightforward as possible, and that selecting functions from a list is not the standard behavior but an advanced one. It should be supported, but not encouraged. It also doesn't really matter if the method exists or not also, the editor will create it in case it doesn't, so having an option for this is pointless.

@samdze
Copy link
Contributor Author

samdze commented Apr 20, 2019

@reduz This new UI is designed to be as simple as possible by default, it is in fact very similar to your mockup when you first open the dialog.

immagine
As you can see, it only gives a little more informations and hints on what you could change, tooltips will do the rest if a user is brave enough to explore.

New users will find connecting a signal to a script as easy as in your concept because, not without reason, the "Existing Method" mode is sorta like your "Advanced..." checkbox.

The mode setting is there for advanced use AND to guide users by hand if they need to, it favors discoverability and it auto-explains what is going to happen.
It doesn't matter to Godot if a method exists or not, but it does to the user, and this dialog gives all the tools needed to understand what is going on and what you need to configure if you want a given result.

This dialog just gives more possibilities and efficiency to those who need it, still being simple to those who see it for the first time and only have to connect a signal to their script.

As for the selectable method list, I received feedback stating that a real time consultation is much more handy and efficient, but showing it all the time only clutters the UI, so I put it in his own mode, there to appear only if needed.

Then this also works well with Editor Features profiles, if your profile has "Script Editor" disabled you are only able to connect the signal to existing methods, this dialog guides you to do just that in this case.

@JrDanny
Copy link

JrDanny commented Apr 21, 2019

Wonderful

@samdze samdze force-pushed the connect-dialog-update branch 2 times, most recently from c3d9e5a to 766caaa Compare April 26, 2019 13:52
@samdze samdze force-pushed the connect-dialog-update branch from 766caaa to 47175df Compare April 26, 2019 14:31
@samdze
Copy link
Contributor Author

samdze commented Apr 28, 2019

I'll create a more unified and simple version of this dialog in the next days, still trying to integrate all the tools needed for an advanced use in a meaningful way.

@reduz
Copy link
Member

reduz commented May 6, 2019

Again, I think this is too complex and does not focus on real life use cases.
I implemented this same thing originally and realized it was useless shortly afterwards 10 years ago.
The only functions that should be shown, and as a small helpers, are those in the script which match the signal arguments, and a small popup is enough for this. You don't connect to any other function in at least 99.9999% of cases, so making the dialog this complex is not the way to go.

@samdze
Copy link
Contributor Author

samdze commented May 6, 2019

@reduz As I wrote above, I'll remake this dialog making it simple but still trying to provide advanced functionality in a way that makes sense and doesn't overwhelm the user.
No more distinct connect modes.

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Nov 7, 2019
@rcorre
Copy link
Contributor

rcorre commented Mar 21, 2020

So is there any interest in possibly adding a PropertySelector to pick the target method?

Maybe I just have a weird workflow, but I find I'm frequently connecting signals in the editor, and that typing in the method name is slow (and sometimes requires going back to read the script to remember the name). A PropertySelector in the dialog (or in a separate dialog, created on a button press?) would be a huge improvement.

I'm also unclear on why the new dialog requires an extra click to activate "Advanced" mode so I can access functionality that was easily accessible in 3.1. It seems that, compared to the proposal here, the 3.2 dialog is very mouse-heavy and cumbersome.

@reduz based on this comment, it sounds like you would be ok with a dialog as long as it filtered the selection down (though given that signals can't be typed, it limits how much we can filter)?

The only functions that should be shown, and as a small helpers, are those in the script which match the signal arguments, and a small popup is enough for this.

(Hope this doesn't sound too critical. ❤️ 3.2, but the new signal dialog is a sticking point in my workflow, and this PR seemed like really good work).

@aaronfranke aaronfranke marked this pull request as draft April 9, 2020 00:36
@aaronfranke
Copy link
Member

@samdze Is this still desired? If so, it needs to be finished and rebased on the latest master branch.

If not, abandoned pull requests will be closed in the future as announced here.

@rcorre
Copy link
Contributor

rcorre commented May 13, 2020

@aaronfranke I'm still interested and happy to finish the work if @samdze does not. However, it seems like the sticking point less about getting the work done and more about agreeing on a UI.

@samdze
Copy link
Contributor Author

samdze commented May 13, 2020

I'd be available to finish this PR sooner or later and I was quite happy with the current implementation. (so, distinct modes with the initial view covering 95% of use cases)

So yes, if this type (maybe not exactly this) of UI is accepted, I'll make the effort.

2019-04-22_16-18-31

If there's still no consensus I'm open to other solutions but I'd like to make this dialog a full fledged tool for signal connections.

@groud
Copy link
Member

groud commented May 13, 2020

For me, this last version looks very good. The UX looks a lot better

@pwnorbitals
Copy link

I like the new design !! Is this still being worked on ? :)

@bodamat
Copy link

bodamat commented May 3, 2021

New design is so good. It looks not too complex. Left dropdown very useful and needed, but right dropdown maybe hide to the args window or add advance button for right functionality. I think we don't use it much, especially for beginners. Thanks for your work!

@samdze
Copy link
Contributor Author

samdze commented May 3, 2021

Is it preferred to keep the separate modes ("Existing Method" and "New Method") to better guide the user or is a unique and unified mode (with a button + popup for existing methods selection) the favourite solution?

@bodamat
Copy link

bodamat commented May 4, 2021

Good idea with 2 buttons and a pop-up window for "Existing Method", but in use, more pop-up windows make inconvenience. Maybe create just a checkbox if the need for the existing method. For default create a new method.
Now we have checkbox "advance". If you move these elements there or in main windows keep only create method mode like checkbox?

@mrbbbaixue
Copy link
Contributor

I like this new design very much!!! Is this still being worked on ?

@Calinou
Copy link
Member

Calinou commented Feb 23, 2022

I like this new design very much!!! Is this still being worked on ?

This pull request needs to be rebased before it can be considered for merging. That said, the current implementation of this PR didn't gather reduz's approval, and that's required for this PR to be merged too.

@akien-mga akien-mga removed this from the 4.0 milestone Jul 26, 2022
@akien-mga akien-mga removed the request for review from reduz July 26, 2022 12:33
@akien-mga
Copy link
Member

Would need a rereview to compare with the current implementation in 4.0 and see what would still be relevant to change. CC @godotengine/docks

@akien-mga akien-mga added this to the 4.x milestone Jul 26, 2022
@KoBeWi
Copy link
Member

KoBeWi commented Jul 26, 2022

I have a similar PR, but it only moves things around: #60478

@KoBeWi
Copy link
Member

KoBeWi commented Sep 22, 2022

I looked at the checklist in the OP:

There are now two distinct modes in the ConnectDialog: "New Method" and "Existing Method" modes.

This would be maybe ok if it was done with CheckButton, but the approach with ... button from #38218 is better.

Existing Method mode lets the user select the method of choice inside a list of available methods. The list can be searched and the methods descriptions can be viewed in real-time.

This is great and still wanted.

Adjustments in the UI of the Dialog: Deferred/Oneshot checkboxes and "Edit Arguments..." button.

The flags are fine as now after #60478
As for the arguments, I think the current advanced mode is ok. No need to change it.

Added arguments info in the main dialog.

This is nice. Could also display argument types (and also warn about mismatch?)

Added locked arguments info in the bindings dialog for arguments passed directly by the emitting signal.

This is interesting 🤔 But probably not necessary with the above.

Children nodes of instanced scenes are displayed in the Connect Dialog SceneTree if Editable Children is active.

This was already implemented in another PR.

The script and instanced scene icons now always appear in the scene tree, with tooltips.

That could be nice to have too.

Disable "New Method" mode if Script Editor is disabled in the current Editor Features profile.

Still relevant.

Overall I'd avoid making too many UX changes. The current advanced mode is fine (aside from the fact that it awkwardly resizes the dialog), because it's easily accessible. The method picker can be left for #38218, so this leaves only a few minor changes that are relevant in this PR. It would be easier to review if it was smaller and didn't change too much.

@YuriSizov
Copy link
Contributor

YuriSizov commented Oct 6, 2023

I'm sorry that this got stuck in a limbo. I think that general direction for UX improvements that this PR proposes is interesting (also see KoBeWi's comment above). But at this point this PR basically needs to be redone due to the amount of code changes since it has been last touched.

I think that parts of it could be considered as good self-sufficient PRs for 4.3, whether by samdze or by other contributors. I'm going to close it, but the salvageable label should remain as a hint that this is still desirable in some form.

@YuriSizov YuriSizov closed this Oct 6, 2023
@YuriSizov YuriSizov removed this from the 4.x milestone Oct 6, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Oct 6, 2023

Note that #66313 was merged in the meantime, so the method picker already exists now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.