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

[Mac] NSView based tables #714

Merged
merged 8 commits into from Jan 26, 2018
Merged

[Mac] NSView based tables #714

merged 8 commits into from Jan 26, 2018

Conversation

sevoku
Copy link
Member

@sevoku sevoku commented Aug 3, 2017

This PR replaces the deprecated NSCell based table/cell implementation with the recommended NSView based table mode.

Features:

  • Makes use of the native NSView caching technique (reusable cell views)
  • Less allocations (only one backend instance for each CellView)
  • Faster row/column size calculations
  • Full CellView event support


public override void MouseEntered (NSEvent theEvent)
{
this.HandleMouseEntered (theEvent);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be checked like the others?

Copy link
Member Author

Choose a reason for hiding this comment

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

do you mean whther it has been handled? The entered/exited events have no arguments and can't be interrupted: https://github.com/mono/xwt/blob/master/Xwt/Xwt.Backends/ICellViewFrontend.cs#L49-L50, hence no need to check here.

public override void MouseDown (NSEvent theEvent)
{
if (!this.HandleMouseDown (theEvent))
base.MouseDown (theEvent);
Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace is off


public override void MouseExited (NSEvent theEvent)
{
this.HandleMouseExited (theEvent);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be checked like the others?


public override void MouseDragged (NSEvent theEvent)
{
if (!this.HandleMouseMoved (theEvent))
Copy link
Contributor

Choose a reason for hiding this comment

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

HandleMouseDragged

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, HandleMouseMoved is right, we have no dragging support (not implemented and disabled) and dragging won't fire the MouseMoved event. We do the same for normal widgets here: https://github.com/mono/xwt/blob/master/Xwt.XamMac/Xwt.Mac/WidgetView.cs#L172-L181

Copy link
Contributor

Choose a reason for hiding this comment

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

As in, shouldn't this return bool?

c12f658#diff-2229d0433bd649eface587819a9c0a4cR125

Copy link
Member Author

Choose a reason for hiding this comment

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

sry, can you explain what you mean exactly? Its an override void and the base is called only if the handler returns false. The other handlers (MouseEntered/MouseExited) have no event args in Xwt and therefore there is no need to check whether they return true/false, the base method will always be called there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I now understand why it's like this. 👍

c.Fill ();
}

var s = CellSize;
foreach (var c in GetCells (new CGRect (new CGPoint (0, 0), Frame.Size))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

CGPoint.Empty here?

int nexpands = 0;
double requiredSize = 0;
double availableSize = cellFrame.Width;
var sizes = new Dictionary<ICellRenderer, double> ();
Copy link
Contributor

Choose a reason for hiding this comment

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

Sizes used to be a dictionary due to having to access by ICellRenderer later on. Can't this be a simple list? Is a cell reusable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hhm, yeah, this could be optimized, I just copied the old code.

Copy link
Member Author

Choose a reason for hiding this comment

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

done: 1c43f37

@slluis
Copy link
Member

slluis commented Aug 7, 2017

It looks good overall. That's a big change, a thorough review will take time...

Copy link
Contributor

@Therzok Therzok left a comment

Choose a reason for hiding this comment

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

LGTM after a quick first pass.

var container = Table.GetView (cellBackend.Column, row, false) as CompositeCell;
if (container != null) {
var cellView = container.GetCellViewForBackend (cellBackend);
rect = cellView.ConvertRectToView (new CoreGraphics.CGRect (new CoreGraphics.CGPoint (0, 0), cellView.Frame.Size), Table).ToXwtRect ();
Copy link
Contributor

Choose a reason for hiding this comment

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

CGPoint.Empty?

@sevoku sevoku merged commit 2ef8261 into master Jan 26, 2018
@alanmcgovern alanmcgovern deleted the mac-nsview-based-tables branch May 31, 2018 16:09
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

6 participants