Lots of small fixes + a much better linearlayout #363

Closed
wants to merge 32 commits into
from

Projects

None yet

3 participants

@csteeg
Contributor
csteeg commented Jul 18, 2013

@Slodge... i again modified the getviewimpl to reuse the correct views. I got much better performance. I really hope to get to the examples project this weekend!

@slodge
Contributor
slodge commented Jul 18, 2013

Thanks

25 changed files with 521 additions and 249 deletions.

How can I view these fixes/changes in action? Are there screens on https://github.com/slodge/MvvmCross-Tutorials/tree/master/DialogExamples (or other) that I can see them and test them?

Understand it's not easy to test this UI stuff and that the inherited codebase is a work of many authors and a bit special. Really don't want to put you off submitting pulls - but would love to work out some way of testing changes are good... very happy to admit I'm struggling a bit to work out how to maintain/QA this imported dialog code

Stuart

@slodge
Contributor
slodge commented Jul 18, 2013

Maybe I can spin this around on you....

If someone else submits a pull in these areas - e.g. to convertview or to the new linearlayout code... what QA do you want MvvmCross to do on their pull before accepting it?

@slodge
Contributor
slodge commented Jul 18, 2013

And now.....

I finally read your pull comment....

I really hope to get to the examples project this weekend!

I am such a doofus

Ignore me...

As you were :)

@slodge
Contributor
slodge commented Jul 22, 2013

Here's a very quick code review set of questions

Hope you don't mind :)

###convertView

  • I'm much happier with these convertView's removed from the call hierarchy and cleared up - thanks.
  • I think there might still be a few issues left behind, but only a couple look like new ones, the rest look like they are detritus from the shady Dialog past! Also... I don't know if any of them matters...

** what is your understanding now of what Adapter.IgnoreItemViewType; ** does? Does this mean we'll never get called with a convertView except our own? If this is the case, then I suspect none of the below points matter...

Beyond this... the possible minor issues are below - but I don't know if any of them matter if cells never get reused:

  • possible new issues

    ButtonElement.cs - where does button.SetOnClickListener(this); get called?
    FloatElement.cs - I think the call to slider.SetOnSeekBarChangeListener(this); in GetViewImpl() should be removed
    StringDisplayingValueElement.cs - I think we now need to move label.TextSize = FontSize; and value.TextSize = FontSize;

  • left over from the past...

    HtmlElement.cs - never used this - Click there feels odd, but I guess is OK
    ImageElement.cs - not sure about the scaled code in GetViewImpl - no idea what this does (I do vaguely know who Kenny is!)
    SectionElement.cs - the TextView block smells odd
    WebContentElement.cs - well, let's just hope this never gets reused...

###DividerAwareLinearLayout

  • no idea without seeing it in action but can accept as-is

###EntryElement

  • what does GetImmediateRootElement do? Who uses it?

###All Dispose changes

  • I much prefer code to do nothing if the bool isDisposing parameter is false - otherwise all bets are off and you shouldn't call Dispose on child objects?
  • Any code that call GC.Collect(0); needs a comment justifying it?

###ResourceIds
The resource id location code in LinearDialogStyleableResource feels like it's been borrowed from the existing MvxAndroidBindingResource code - do we need to make this shared somehow?

@csteeg
Contributor
csteeg commented Jul 24, 2013

** what is your understanding now of what Adapter.IgnoreItemViewType; ** does? Does this mean we'll never get called with a convertView except our own? If this is the case, then I suspect none of the below points matter...

My understanding is as follows:

  • If you return Adapter.IgnoreItemViewType, you always get the cell that is currently displayed at that cell position.
  • So we only have to check if convertView is not the one we previously returned (if the elements collection has changed, it could be that another type of view should be returned now)

ButtonElement.cs - where does button.SetOnClickListener(this); get called?
I don't get this one? :) It's still in GetViewImpl, did not change that
ImageElement.cs - not sure about the scaled code in GetViewImpl
In my project, I've created my own ImageElement, since I needed it to be a stringvalue element. Indeed there were some issues with the current ImageElement. I will try to merge that code into your imageelement
I also removed the static ImagePicker in my own code, I did not encounter any memory problems because of that. Is this still an issue?

FloatElement.cs - I think the call to slider.SetOnSeekBarChangeListener(this); in GetViewImpl() should be removed
Indeed, done

StringDisplayingValueElement.cs I think we now need to move label.TextSize = FontSize; and value.TextSize = FontSize;
Moved them...

what does GetImmediateRootElement do? Who uses it?
sorry: left over from earlier code changes. Removed it

###All Dispose changes
Fixed those

LinearDialogStyleableResource
This is indeed borrowed from MvxAndroidBindingResource.....

@slodge
Contributor
slodge commented Aug 23, 2013

Hi @csteeg

Back from holidays and integrating code again...

On this, I'm still trying to work out what the test harness should be.

I'm especially concerned about the 'deep stuff' like convertview but also want to somehow check on the UI stuff too - e.g. if there are animation changes then how can someone check these are working?

Still thinking about:

If someone else submits a pull in these areas - e.g. to convertview or to the new linearlayout code... what QA do you want MvvmCross to do on their pull before accepting it?

Stuart

@slodge
slodge commented on 0ed7de8 Aug 23, 2013

Not sure we want to add IElementSizing to every Element because of efficiency trick.

See @migueldeicaza's comment on https://github.com/migueldeicaza/MonoTouch.Dialog/blob/master/MonoTouch.Dialog/DialogViewController.cs#L437

// Performance trick, if we expose GetHeightForRow, the UITableView will
// probe *every* row for its size; Avoid this by creating a separate
// model that is used only when we have items that require resizing
@csteeg
Contributor
csteeg commented Aug 24, 2013

Hi Stuart,

I'm not back yet. I really did not get into providing samples before my
holiday, sorry for that.

However, the app we needed all the changes for, is in the stores now.
It's in Dutch only, for a Dutch healthcare insurance company. You can
find for Android and iPhone and it's called zorgnotities.

I have another week here on my holiday, will see how soon the planning
will allow me to provide some samples.

Regards,
Chris
Op 24 aug. 2013 01:06 schreef "Stuart Lodge" notifications@github.com het
volgende:

Hi @csteeg https://github.com/csteeg

Back from holidays and integrating code again...

On this, I'm still trying to work out what the test harness should be.

I'm especially concerned about the 'deep stuff' like convertview but also
want to somehow check on the UI stuff too - e.g. if there are animation
changes then how can someone check these are working?

Still thinking about:

If someone else submits a pull in these areas - e.g. to convertview or to
the new linearlayout code... what QA do you want MvvmCross to do on their
pull before accepting it?

Stuart


Reply to this email directly or view it on GitHubhttps://github.com/slodge/MvvmCross/pull/363#issuecomment-23184095
.

@slodge
Contributor
slodge commented Oct 5, 2013

The tip of BindingChanges is now up to date with this PR :)

I added some small changes to switch some private fields to protected properties - see d3dcef5 - if more is needed on this let me know... I just want to stop people making fields protected if I can :)


I do feel the need to ask about these lines added to Visible in Element.cs

            if (CurrentAttachedCell != null && CurrentAttachedCell.Superview is UITableView)
            {
                var indexPath = ((UITableView)CurrentAttachedCell.Superview).IndexPathForCell(CurrentAttachedCell);
                if (indexPath == null)
                {
                    UpdateCellDisplay(CurrentAttachedCell);
                }
                else
                {
                    ((UITableView) CurrentAttachedCell.Superview).ReloadRows(
                        new[] {indexPath},
                        UITableViewRowAnimation.Fade);
                }
            }
            else
            {
                UpdateCellDisplay(CurrentAttachedCell);
            }

This strikes me as quite a complicated set of if statements - and I couldn't really understand why each branch would occur?

e.g. when should CurrentAttachedCell.Superview is UITableView be false?
when should IndexPathForCell return null?


Thanks again for all the work - I'd still like to work out how to test this - especially things like how to test the Droid convertView code - but in the absence of any existing tests, I'd rather have your new code there than the old stuff - you seem to know what you are doing and seem to be doing it very skillfully :)

Stuart

@slodge
Contributor
slodge commented Oct 5, 2013

Testing today :)

Mostly seemed to work in DialogExamples.

Have some issues with the Date Time and Radio Pickers - they don't seem to activate. Probably an issue with the way I've merged. Will hopefully fix tomorrow!

@slodge
Contributor
slodge commented Oct 6, 2013

I've tested some more and the problem seems to be that ItemClick isn't firing at all on the MvxDialogActivity - so things like the datetime picker just aren't showing

I've tracked this down to the recent Clickable changes in Element.cs - if I remove the Clickable and LongClickable cell calls then the radio button and date time pickers just start working

    protected virtual void UpdateCellDisplay(View cell)
    {
        if (cell == null)
            return;

        //cell.Clickable = Clickable.HasValue ? Clickable.Value : true;
        //cell.LongClickable = LongClickable.HasValue ? LongClickable.Value : true;

I'd quite like to solve this so I can ship 3.0.13 with the dialog code in - but I'm not sure what the impact is if I change these Clickable defaults to false

If you get a minute can you take a look and let me know what you think we should do?

Thanks (Sorry for bothering you on the weekend - and for taking so long to do these merges!)

Stuart


I also found this code in public void AddViews() in LinearDialogScrollView - suggest it can be removed :)

            view.Click -= ListView_ItemClick;
            view.LongClick -= ListView_ItemLongClick;
            view.Click += ListView_ItemClick;
            view.LongClick += ListView_ItemLongClick;
@slodge
Contributor
slodge commented Oct 6, 2013

Here's what I've checked back in - 8a3c2c1 - but don't know if this is going to break other things...

@csteeg
Contributor
csteeg commented Oct 7, 2013

Hi Stuart,

Sorry for my late response. I was not able to look at your questions and
merges last weekend, sorry for that.

I'll have a look at it first thing tomorrow!

Regards,
Chris

Here's what I've checked back in -
8a3c2c18a3c2c1
but don't know if this is going to break other things...


Reply to this email directly or view it on
GitHubhttps://github.com/MvvmCross/MvvmCross/pull/363#issuecomment-25766859
.

@csteeg
Contributor
csteeg commented Oct 8, 2013

Hi Stuart,

the clickable as false indeed works as you describe.
The only thing it breaks as far as I can see, are entries with focusable elements to get focus on click. So I committed csteeg@5d4a9ce wich has Clickable set to true on EntryElements (made it play a little nicer in csteeg@f85a643 :))

the code from public void AddViews() in LinearDialogScrollView cannot go, since the adapter cannot attach to the ScrollView's itemclick. It contained almost the same code as the dialogadapter's ItemClick events, so I modified the scrollview to reuse that code.

Regards,
Chris

@slodge
Contributor
slodge commented Oct 8, 2013

Thanks Chris

Will try to look at this tonight - will test and maybe even release :)


I don't really understand that code from AddViews() though:

From https://github.com/MvvmCross/MvvmCross/blob/BindingChanges/CrossUI/CrossUI.Droid/Dialog/LinearDialogScrollView.cs#L158 the 4 lines are:

        view.Click -= ListView_ItemClick;
        view.LongClick -= ListView_ItemLongClick;
        view.Click += ListView_ItemClick;
        view.LongClick += ListView_ItemLongClick;

Doesn't this just remove, and then add the same handlers? Why not just leave the existing handlers in place? If it should stay, what comment could we put ahead of it so that no-one else ever removed it? Or maybe we should put it in a function like EnsureViewClickAndLongClickSubscribed(view) ?

@csteeg
Contributor
csteeg commented Oct 8, 2013

The code is there, because AddViews can get called several times (eg if the bound collection changes, it adds all views again).
Since the views are re-used incase they were created once already, the events are still attached at this point. So I detach my own subscription and re-attach it. As far as I know, this is the only way to avoid subscribing to the same event twice (http://stackoverflow.com/questions/937181/c-sharp-pattern-to-prevent-an-event-handler-hooked-twice)

An EnsureViewClickAndLongClickSubscribed(view) would be just fine by me as well.

@slodge
Contributor
slodge commented Oct 8, 2013

Absolutely fascinating. Thank you for teaching me stuff :)

Just had a play in LinqPad and this code taught me that you can definitely call -= without calling += and that you can call += multiple times and -= will remove just one each time - I didn't know any of that :)

Thanks!

Will try to get back to work now - then look at this tonight :) Might even get to close this pull request :)

void Main()
{
    var t= new Thing();
    var s = new Sub(t);
    t.Raise();
}

// Define other methods and classes here
public class Thing
{
    public event EventHandler Go;

    public void Raise()
    {
        Go(this, EventArgs.Empty);
    }
}

public class Sub
{
    public Sub(Thing t)
    {
        t.Go -= Handler;
        t.Go += Handler;
        t.Go -= Handler;
        t.Go += Handler;
        t.Go += Handler;
        t.Go -= Handler;
    }

    public void Handler(object sender, EventArgs e)
    {
        "Handling".Dump();
        sender.Dump();
    }
}
@csteeg
Contributor
csteeg commented Oct 8, 2013

Now for the other question, about this code:

if (CurrentAttachedCell != null && CurrentAttachedCell.Superview is UITableView)
            {
                var indexPath = ((UITableView)CurrentAttachedCell.Superview).IndexPathForCell(CurrentAttachedCell);
                if (indexPath == null)
                {
                    UpdateCellDisplay(CurrentAttachedCell);
                }
                else
                {
                    ((UITableView) CurrentAttachedCell.Superview).ReloadRows(
                        new[] {indexPath},
                        UITableViewRowAnimation.Fade);
                }
            }
            else
            {
                UpdateCellDisplay(CurrentAttachedCell);
            }

It indeed is quite a bunch of code just to be able to animate the visibility toggling, but it is there for a reason.
Indexpath got null sometimes as I was replacing content of a list by setting a new RootElement.
It's a really rare situation when you perform several actions on a listview and it's busy animating stuff.

SuperView should indeed never be anything else then a UITableView but since I used the strong conversion in ((UITableView)CurrentAttachedCell.Superview) I thought it'd be nice to check first.

@slodge
Contributor
slodge commented Oct 8, 2013

I've pushed these changes - will test tonight - then maybe we can close this big pull request :)

Thanks for all the input - fab stuff :)

@slodge
Contributor
slodge commented Oct 8, 2013

Oops - forgot to include link to my fixes - according to your suggestions - here they are e79beb2

@slodge
Contributor
slodge commented Oct 8, 2013

OK - everything's built and initial test seems to work - so I'm closing this one :)

Thanks again for all the fish - top coding - thanks :)

Stuart

@slodge slodge closed this Oct 8, 2013
@tofutim tofutim pushed a commit to loqu8/MvvmCross that referenced this pull request Nov 12, 2013
@slodge slodge Fixes for MvvmCross#363 - could these be the last for 3.0.13 :) e79beb2
@zleao zleao pushed a commit to zleao/MvvmCross-Tutorials that referenced this pull request Jan 22, 2014
@slodge slodge Dialog Examples - hack to make the sample work - see comments on Mvvm… f36f617
@martijn00 martijn00 added this to the 3.0.x milestone Jan 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment