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

MacOS #650

Merged
merged 284 commits into from Jan 26, 2017
Merged

MacOS #650

merged 284 commits into from Jan 26, 2017

Conversation

rmarinho
Copy link
Member

@rmarinho rmarinho commented Dec 15, 2016

Description of Change

This PR adds support for MacOS backend on Xamarin.Forms.

All the controls are implemented expect OpenGLView, but not all features are present.
We have UITest rudimentary support on CI.

Opening the PR to prepare for review and merge in the following weeks. As well as get general feedback.
Take in mind most renderers are ported from the iOS counterpart and might have some incongruences that we can now fix in this new implementation , please point them out if see them.

Xamarin.Forms.Maps.MacOS.Extra is a extra lib that is only needed to support Cycle 8, it has some missing APIS that were only added to Xamarin.Mac in Cycle9.

TODO:
Missing:
OpenGlRenderer
ListView - ScrollTo, Unevenrows, refreshing, Separator color and visibility, footer
ViewCell - is enable missing, onforce update size 
Image - Aspect
Picker - Bindable/ observable implementation
MasterDetailpage- Background color missing
tabbedPage - BarBackgroundColor and BarText Color
TableView - Unevenrows
WebView - Most WebNavigationEvents
Navigation - insert page before

In alpha, needing feedback, basicand modal navigation, UI and styling of controls, we welcome pr's 

Behavioral Changes

Add MacOS backend

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

@rmarinho rmarinho removed the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Jan 21, 2017
@@ -13,6 +13,7 @@
[assembly: AssemblyConfiguration("")]
[assembly: AssemblyCulture("")]
[assembly: NeutralResourcesLanguage("en")]
[assembly: InternalsVisibleTo("Xamarin.Forms.Maps.macOS")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:(

var textCell = (TextCell)tvc.Cell;
if (args.PropertyName == TextCell.TextProperty.PropertyName)
{
tvc.TextLabel.StringValue = ((TextCell)tvc.Cell).Text ?? "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you use textCell here instead of casting again?

}
else if (args.PropertyName == TextCell.DetailProperty.PropertyName)
{
tvc.DetailTextLabel.StringValue = ((TextCell)tvc.Cell).Detail ?? "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you use textCell here instead of casting again?


internal NSViewController ViewController => PlatformRenderer;

internal static void DisposeModelAndChildrenRenderers(Element view)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's really Model like yourself and children (this is the same on iOS)

if (rendererToRemove == null || rendererToRemove.Element == null)
return;

if (rendererToRemove.Element != null && GetRenderer(rendererToRemove.Element) == rendererToRemove)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already null check rendererToRemove.Element just above this.


ITemplatedItemsView<Cell> TemplatedItemsView => Element;

public const int DefaultRowHeight = 44;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This says 16 is the default row height for macOS.

_disposed = true;
if (Element != null)
{
//((ObservableList<string>)Element.Items).CollectionChanged -= RowsCollectionChanged;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a todo?


protected override void SetBackgroundColor(Color color)
{
//base.SetBackgroundColor(color);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a todo?

[Export(nameof(UpdateScrollPosition))]
void UpdateScrollPosition()
{
System.Diagnostics.Debug.WriteLine("need to fix this math");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be a todo instead of a debug writeline?

else
{
Control.StringValue = (string)values[1] ?? "";
// default value of color documented to be black in iOS docs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about mac docs? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum.. didn't found it.

@samhouts
Copy link
Member

I'd recommend putting TODOs on all commented code to make it easier to find.

@rmarinho rmarinho merged commit 52fc047 into master Jan 26, 2017
knocte added a commit to knocte/Xamarin.Forms that referenced this pull request Apr 17, 2017
rmarinho pushed a commit that referenced this pull request Apr 18, 2017
@rmarinho
Copy link
Member Author

@samhouts you were so right and i forgot , but did it now :)

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

Successfully merging this pull request may close these issues.

None yet

8 participants