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

Various code logic corrections (without API changes). #198

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
5 participants
@Hybris95
Copy link

commented May 27, 2016

Description of Change

Corrections after passing a PVS-Studio on the Project
No tests because no test environment yet just newly forking the project.

Some corrections may be implemented later (View.cs & LayoutAlignmentExtensions.cs).
Tests needs to be done also.

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

Hybris95 added some commits May 25, 2016

Various code logic corrections.
Some corrections may be implemented later (View.cs & LayoutAlignmentExtensions.cs).
Tests needs to be done also.
@dnfclas

This comment has been minimized.

Copy link

commented May 27, 2016

Hi @Hybris95, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@Hybris95

This comment has been minimized.

Copy link
Author

commented May 27, 2016

Last PR : #194

@@ -88,5 +89,5 @@ static bool ValidateItemTemplate(BindableObject b, object v)

return !(lv.CachingStrategy == ListViewCachingStrategy.RetainElement && lv.ItemTemplate is DataTemplateSelector);
}
}
}

This comment has been minimized.

Copy link
@jassmith

jassmith May 27, 2016

Contributor

Random whitespace change?

@@ -14,8 +14,9 @@ public static double ToDouble(this LayoutAlignment align)
return 0.5;
case LayoutAlignment.End:
return 1;
}
throw new ArgumentOutOfRangeException("align");
default:// TODO - Maybe there is a specific value for LayoutAlignment.Fill ?

This comment has been minimized.

Copy link
@jassmith

jassmith May 27, 2016

Contributor

Incorrect idnentation

@@ -355,5 +355,5 @@ void UpdateCurrentPage()
else if (SelectedItem is T)
CurrentPage = (T)SelectedItem;
}
}
}

This comment has been minimized.

Copy link
@jassmith

jassmith May 27, 2016

Contributor

Same

@@ -455,7 +455,7 @@ public void EnterReadLock()
while ((_rwlock & (RwWrite | RwWait)) > 0)
sw.SpinOnce();

if ((Interlocked.Add(ref _rwlock, RwRead) & (RwWait | RwWait)) == 0)
if ((Interlocked.Add(ref _rwlock, RwRead) & (RwWrite | RwWait)) == 0)

This comment has been minimized.

Copy link
@jassmith

jassmith May 27, 2016

Contributor

+1

@@ -49,7 +49,9 @@ protected internal View()
foreach (IElement item in _gestureRecognizers.OfType<IElement>())
item.Parent = this;
break;
}
case NotifyCollectionChangedAction.Move:

This comment has been minimized.

Copy link
@jassmith

jassmith May 27, 2016

Contributor

whitespace again

This comment has been minimized.

Copy link
@jassmith

jassmith May 27, 2016

Contributor

Also in the case of move this can just be a blank no-op

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix May 30, 2016

Member

GestureRecognizers is exposed as a IList, so a Move is very unlikely to happen.

@@ -1302,6 +1302,9 @@ void OnItemsListCollectionChanged(object sender, NotifyCollectionChangedEventArg
case NotifyCollectionChangedAction.Replace:
e = new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, ConvertItems(e.NewItems), ConvertItems(e.OldItems), e.OldStartingIndex);
break;
case NotifyCollectionChangedAction.Reset:

This comment has been minimized.

Copy link
@jassmith

jassmith May 27, 2016

Contributor

more whitespace

}

if (xIsProportional)
}

This comment has been minimized.

Copy link
@jassmith

jassmith May 27, 2016

Contributor

whitespace :(

@jassmith

This comment has been minimized.

Copy link
Contributor

commented May 27, 2016

Changes to absolute layout break unit tests

}
else
{
// Only width is auto

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix May 30, 2016

Member

looks like we're loosing a case here when !heightIsProportional

@rmarinho

This comment has been minimized.

Copy link
Member

commented Jun 8, 2016

Please re-open when addressing comments, coding style, and unit tests failures.

@rmarinho rmarinho closed this Jun 8, 2016

@Hybris95

This comment has been minimized.

Copy link
Author

commented Jun 11, 2016

Please do it yourself, I have no interest doing so. Fix yourself the bugs I pointed out if you wish to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.