[Mac] Initial Binding Editor Dialog.

[CartBlanche]

Fixes: https://github.com/xamarin/Xamarin.PropertyEditing/issues/284

[CartBlanche]

@vancura Please cast your design eye over these screenshots...

[vancura]

A couple of comments:

• [x] Please use different space around the window, your is too low
• [x] Also space between top dropdown section and two tables
• [x] The Path / Custom checkbox needs to be centered
• [x] Left panel has no header (in my comp it writes "Ancestor Type and Level")
• [x] The list on the right (your "MockSampleControl") shouldn't have a border around, it should be edge to edge
• [x] Space between two panels and the Converter section is too low
• [x] Other Settings should always be named Other Settings, even when it's expanded (you have Hide Settings in that case)
• [x] Please vertically center the Other Settings panel contents, it's too much on top.

[vancura]

(left my design, right your screenshot)

• [x] Different padding around the window
• [x] Filter and the MockValueConverter don't horizontally align
• [x] Done button shouldn't be aligned with the Show all assemblies checkbox
• [x] Show All Assemblies should be capitalized
• [x] Why only Done button?

Do you show the helper text when there's nothing listed?

[ermau]

Left panel has no header (in my comp it writes "Ancestor Type and Level")

It actually does have a header, you can see it in the second screenshot. It's dependent on the selected binding type.

[vancura]

It actually does have a header, you can see it in the second screenshot. It's dependent on the selected binding type.

Can we display it at all times?

[ermau]

Can we display it at all times?

I don't know that it makes sense to show it at all times. What would we show in the header for the scenario in the current screenshot? It's essentially a call to action and there isn't always an action on the left hand side.

[vancura]

@ermau: Ok, fair enough :)

[CartBlanche]

https://github.com/xamarin/Xamarin.PropertyEditing/pull/561#discussion_r268811108 @ermau Not sure what you mean by "marked as resolved" and not fixed here?

[CartBlanche]

@vancura Can you please cast your eye over these screen shots.

I'm investigating why the More properties Flags isn't respecting the constraints, but think I've addressed the issues you pointed out?

[vancura]

Your redline 🐒 says hi.

Basic Window Metrics

• [x] Spacing around the window edge should be 20pt, you have 29pt.
• [x] Vertical space between sections should be 23pt, you have 28pt.
• [ ] Please make the cancel button wider, specify minimum size to 80pt.
• [x] No help button? Just asking, if we don't have the contents, no worries. But help is always good.

Header Section

• [x] Use standard-sized dropdown (it seems you use tiny style).
• [x] Title font size should be 13pt, you seem to have 11pt.
• [x] Missing colon and capitalization: you have Binding type istead of Binding Type:.

Table Section

• [x] Path vs. checkbox header should use 20pt space, but uses circa 60pt.
• [ ] Please use standard line height in the tree view, lines are too separated.

Bottom Section

• [x] The title Converter is missing a colon (should be Converter:) and follow the same font size as specified above, in the header section (13pt).
• [x] Space between dropdown and the plus button should be 10pt, you have something around 136pt (seems it's due to right-alignment of the plus button and automatic sizing of the dropdown; instead the dropdown needs to be fit the whole width.
• [x] Contents of the bottom part should be vertically centered.

Table Margins

• [x] Please remove this padding, it shouldn't be there.

Type Selector Window Metrics

• [x] Spacing around the window edge should be 20pt, you have 9pt.

Other Selector Window Inconsistencies

• [x] Use standard filtering control.
• [x] Align button set with the contents.

Your redline 🐒 thanks and says bye.

[CartBlanche]

@vancura Here are some updated screenshots, based on using Figma to double check spacing and heights. These aren't 100% complete, but want to post these for you check before I push more pixels around.

I don't think the EditBox in the PathSelector looks good flush with box edge.

I don't think we can make the type selector flush to the box, as it would clip the Show All Assemblies checkbox, on the left.

[CartBlanche]

@vancura

Please use standard line height in the tree view, lines are too separated.

We are using the default height for each tree node.

No help button? Just asking, if we don't have the contents, no worries. But help is always good.

No help button for now. This may be added in future.

[vancura]

This is looking amazing, well done @CartBlanche!

---

I don't think the EditBox in the PathSelector looks good flush with box edge.

Can you try something like this? I removed the gap between the EditBox and the PathSelector, and added some space around the label inside the EditBox.

---

I don't think we can make the type selector flush to the box, as it would clip the Show All Assemblies checkbox, on the left.

Can we have something like this?

Changes:

• [x] unified size of both EditBoxes in headers,
• [x] removed the gap between EditBoxes and the selectors (see above),
• [x] removed horizontal gap between the left selector and the edge of the table,
• [x] moved the Show All Assemblies slightly right to accommodate for lost space,

You said:

We are using the default height for each tree node.

Well, in that case why both these selectors have different node height? From what I see here they contain a similar type of information, so they should have the same metrics. When next to each other it's odd how the node height differs.

---

A couple of changes:

• increased horizontal space between the right edge of the window and the right edge of the contents by 4pt, so it's 20pt as everywhere else,
• decreased vertical space between the top edge of the window and the top edge of the Value converter name by 4pt, so it's 20pt as everywhere else,
• (I didn't do this as my SF fonts are broken now): please reword Value converter name to Value Converter Name: (capitalization and colon on end),
• moved buttons set, so the buttons are 20pt from the edge of the window,
• changed the width of the buttons to 80pt, make sure the width is always at least this size (of course if verbose languages don't fit, it scales, but 80pt needs to be minimum),
• please, make sure the Select button is default (blue) once a type is selected (so it's possible to press enter to confirm the selection),
• increased the vertical gap between button set and the Show All Assemblies checkbox by 12pt,
• optional: if it's possible to get the filter EditBox and the selector close to each other, similar to the screenshots above, this would be a killer.

[CartBlanche]

@vancura I think these new changes look good....

As we are reusing the Type Selector Control in the CreateValueConvertorWindow the checkbox will be offset.

[vancura]

This is awesome! Thanks for all the hard work @CartBlanche!

[CartBlanche]

@ermau Yes It was a subscription issue and I wasn't setting the Appearance when creating the BindingDialog or the ValueConvertor Dialog. Both fixed now.

[CartBlanche]

@ermau I'm not seeing this behaviour. The only thing that has been a theme bug for a while is the fact the wrong tab is on by default. This isn't related to the Binding Dialog change as it was around before then. I've pushed a fix for the tabs to be properly synced, as that needed fixing anyway.

[CartBlanche]

@ermau could you please post a video of the behaviour you are seeing?

[ermau]

I don't have a screen recorder on mac, but here are my exact steps:

1. Scroll down to the "String" property, create binding
2. Cancel create binding dialog
3. Switch to dark theme
4. Create binding on "String"

You will get the light themed dialog against dark:

Then when you close that dialog you'll get the dark themed one:

[CartBlanche]

I see it now. Very odd. Investigating.

[CartBlanche]

Right, events not being unsubscribed correctly. Appears to be fixed now.

[CartBlanche]

OK Tested both scenarios this time. Think I have the right fix for it. Doing a little more testing.

[CartBlanche]

@ermau I'm unable to find an elegant solution to the event subscription issue. Only found hacks, which I don't want to push as they weren't working 100% either. Can't see the woods for the trees anymore. Needs a new pair of eyes, when you have a sec.

[ermau]

@CartBlanche You needed to dig deeper. Your current implementation of the event subscription (in the ViewModel setter) is the correct one; the actual issue is separate.

We know that we're getting two dialogs, so we check for double subscription. We see one subscription when the property shows up, and then we see another when the theme changes. What we don't see is an unsubscribe at any point. Combine that with that one dialog is the old theme and we can deduce that when we change the theme, the property buttons are being re-created (as the old one with the old theme is still firing the event.) Now, that's an issue by itself, we should be reusing them when possible but since we may not always be able to, we need to ensure old discarded ones aren't still attached to anything by setting its View Model to null. A change was made for editors a while back to ensure this, but when property button moved around it was not included, I've comitted the fix to your branch.