This repository has been archived by the owner. It is now read-only.

TUIRefreshControl #89

Closed
wants to merge 23 commits into
from

Conversation

Projects
None yet
6 participants

Yay! It's here! I had to do some crazy magic to get it to work with TUIScrollView and TUITableView. With a bit of reworking, it doesn't have to be dependent on TUITableView- and that's coming soon. It's based on an open-source iOS control called ODRefreshControl by Sephiroth87. Eventually, I hope to factor the origins out and rewrite it, but it could not have been possible without this control's help.

What else is coming soon? No more external dependency on TUIActivityIndicatorView. How? I'll be using a CALayer for that with a block for refresh begin and refresh end updates, and a common drawing block. No need to subclass! What about the control's "slime" itself, you ask? Well, I'll add in the same set of blocks for customization to allow custom "slime".

This works in a fairly straightforward way- it's already demoed in the Example app. Just give it a tug, and it'll do its thing.

This is unrelated, but I think I finally got the hang of git! Hurrah!

Please fix the indentation and merge from master. I don't want to start reviewing until those things have been taken care of, as they will completely change the diff.

Fixed indentation and merged with master. :]

CodaFi commented Sep 25, 2012

Can we get some kind of shout out to @Sephiroth87, who's code looks a whole awful lot like @galaxas0's

Yeah, that's not cool. This absolutely needs copyright attribution, and will not be included without it.

Ouch. Nice catch @CodaFi. Might be good to get in touch with @Sephiroth87 and see if he's okay with this being ported over with attribution.

Oh, I'll definitely do that. I'll message him about this now. Sorry for forgetting the most important part... :(

@galaxas0 I've already done so. No need to ask him again.

Added the all-important credits.
Note to self: must not forget this.

I'll hold off on merging this until someone gets a reply from @Sephiroth87. It's technically legal to merge in, but I'd like him to be personally cool with it.

Technically his credits say that we should buzz him if it's an app, and
@jwilling sent an email, so in due time things will work out. Again, very
sorry about forgetting to credit him.

I've just gotten 👍 from @Sephiroth87. With attribution this should be good to use.

Great! Again, sorry for not attributing him. I will rewrite it to avoid
infringement in the future.

On Tuesday, September 25, 2012, Jonathan Willing wrote:

I've just gotten [image: 👍] from @Sephiroth87https://github.com/Sephiroth87.
With attribution this should be good to use.


Reply to this email directly or view it on GitHubhttps://github.com/github/twui/pull/89#issuecomment-8862624.

Aditya.

Please merge from master. The pull request status is still listed as unmergeable.

Thanks for letting me know and for attributing :)
Anyway, don't feel forced to rewrite it if there's no need to, it's not really an infringement given the license I used ;)

Thanks so much! (By the way, dat logic for the slime… took me a while to figure that out.)

@galaxas0 I don't know whether this is your implementation's fault or whether this is the fault of the original control, but the top of the slime does not stay perfectly aligned with the top of the bounds. When you move it quickly, it seems to jerk down for a split second then jerk back up. This probably is due to a very small delay in the KVO implementation. The bounds are changed before the UI is updated, thus leading to the jerk effect. Not sure if this can be fixed or not, but it's a little disconcerting to see it jerk around like that.

It might be my implementation because TwUI really borked up the TUIScrollView somehow.... I switched to using pullOffset and bounceOffset while registering for contentOffset changes. I'll be looking into these issues soon. Let me get all the other PRs out of the way first :P

CodaFi commented Sep 25, 2012

That and the arrow is rendering upside down...

iOS renders with a flipped coordinate system, so I'll have to translate that arrow.

git tells me everything is up to date....? EDIT: figured it out.

CodaFi commented Sep 26, 2012

@galaxas0 Before you consider this control good to merge, I suggest you take a look over the changes in this gist, which fixes (somewhat) the flippedness, and makes the control "follow" it's table instead of letting the table slide over it (exactly like what the iOS one does), and improves the "goop shooting" effect.

Thanks @CodaFi ! It has a glitch or two left to fix, but it's more than ready to merge right now, @jwilling / @jspahrsummers .

@jwilling jwilling and 1 other commented on an outdated diff Sep 26, 2012

lib/UIKit/TUIRefreshControl.m
+ self.activity.bounds.size.width, self.activity.bounds.size.height);
+ [self.activity startAnimating];
+ }];
+
+ self.refreshing = YES;
+ [self sendActionsForControlEvents:TUIControlEventValueChanged];
+
+ CGPathRelease(toPath);
+ CGPathRelease(shotPath);
+ }
+
+ CGPathRelease(path);
+ CGPathRelease(arrowPath);
+}
+
+@end

Couple of things hopefully we can fix before this is merge.

  • Currently, it's possible to refresh just by getting enough velocity while scrolling. This isn't how PTR is designed. Scroll velocity shouldn't ever trigger a refresh.

Take a look at a side-by-side comparison of iOS and this implementation:

iOS Mac

There are many inconsistencies here that are somewhat bothersome.

  • When initially pulling on TUIRefreshControl, the gray item tracks with the scrolling. On iOS it stays fixed and is revealed.
  • On iOS, the slime collapses into a single dot. On TUIRefreshControl, the shape becomes distorted and is flown up out of sight.
  • When the scroll view is released by the user on TUIRefreshControl, the refreshing control tracks up to the top of the margin of the scroll view until it goes back to where it was sitting before. On iOS, it remains a fixed distance from the top of the screen.

Hopefully these can be addressed. It's close, but it's not quite ready yet!

  • "gray item"...? Would that be the arrow or the slime?
  • The collapsing into a single dot distortion you encountered was the CAAnimation on the shape property going haywire. I'm not too good with CAShapeLayer and I don't exactly know how to fix this yet. I think it's because the path it's ending at is flipped, and so it does a U-turn of sorts into that.
  • I'm working on that right now. That can be fixed easily in my view.

The "Gray item" is the gray circle and the arrow contained within it.

  • Alright, I can fix that pretty soon too, then.
  • I think I can fix this: the end path looks wonky.
  • I've fixed this!

avaidyam added some commits Sep 26, 2012

Fixed activity indicator anchoring.
It now anchors to the top of the scroll view, not a fixed distance.

The refresh slime now slides up to the pulsing dots and hides. I can't get it to anchor well because I'm not too well-versed with how CALayers and CATransactions work. Perhaps someone else could help me with this?

Wish I had more time to take a look at this, but unfortunately I don't. Another problem I've noticed is that the refresh control consistently does not show on the very first pull. On the second pull and thereafter, it shows.

That is because of the example project code triggering a refresh manually.
There is no pullOffset or bounceOffset at that time.

Will this be merged yet, or not, because of the incorrect functionality?

The known issues should be fixed before this is merged in.

@jwilling

When initially pulling on TUIRefreshControl, the gray item tracks with the scrolling. On iOS it stays fixed and is revealed.

Actually, the refresh control slide with the scrollview from the top of the screen, until it reaches the final position and anchors there. You can't really see it in that animation.

@galaxas0
I think the problem with the collapsing animation is your toPath (line 242).
The path to which you morph into should have the same number and typer of segments as the initial one for the best effect.
In this case, the initial shape has 2 arcs and 2 curves while to toPath has only one ellipse.
Also, you'll maybe need to flip the coordinates here too...

@Sephiroth87

Actually, the refresh control slide with the scrollview from the top of the screen, until it reaches the final position and anchors there.

Yeah, you're correct. Looked around a bit at some videos and it seems to be the case. 👍

Another problem still unresolved is that you're able to refresh just by getting enough velocity going upwards. Definitely the PTR view should show, but if the user isn't purposefully dragging down on the scroll view it should not refresh.

CodaFi commented Oct 6, 2012

New problem: The control does not seem to auto-resize with grace. It looks as though either my frame math, or the autoresizingmasks are off on this thing. It also appears to be resetting itself upon autoresizing, which, during a reload, causes some wild contentInset issues.

avaidyam commented Oct 6, 2012

EDIT: I've fixed the slime squashing into the activity indicator. It's fairly "smooth" now. I still need to fix the resizing conflicting with contentInset issues.

Collapse refresh slime onto activity indicator.
More or less accurate-- there has to be a better way, but this works
fine as it is.

avaidyam commented Oct 7, 2012

As far as I can see, every method used to restrict refreshing caused from high velocity scrolling interferes with the whole refresh animation and drawing. This is mostly because of TUIScrollView, and not the refresh control itself. Trying to work around this would require additions and modifications to the scroll view class, fundamentally. @jwilling

avaidyam commented Oct 7, 2012

@CodaFi I don't believe it's resetting itself-- it's going incognito. I think it's afraid of being resized by big brother scroll view.

avaidyam commented Oct 7, 2012

@CodaFi The refresh issue also has to do with TUIScrollView. I've triple-verified everything pertaining to this class alone, and everything is in order. The scroll view decides to muck the content insets up when the view is resized, perhaps because of its layer class.

CodaFi commented Oct 7, 2012

@galaxas0 That was some fine debugging, my friend. It works quite nicely except for the obvious reload problems.

avaidyam commented Oct 7, 2012

Haha, thanks! It took quite some time, as you can tell. =feels like the Einstein of refresh controls= :P

jwilling commented Oct 7, 2012

@galaxas0 Hmm. Something about it still doesn't look right. The slime doesn't collapse into a circle like it should. Any thoughts on this?

avaidyam commented Oct 7, 2012

Actually, I've confirmed it at a pixel level using a magnifying tool. (An app, not a real one, lol). It collapses into a circle, which makes up the same area as the activity indicator. I slowed it down too, and it still works. The animation's a bit odd, but it does collapse. The activity indicator is off, if anything. Its frame setting needs to be relocated and heck, the whole indicator has to be redone- it just doesn't fit.

I sent @galaxas0 the correct collapsing animation, for the other issues, I can try and give a hand when I have some time, but I never actually done any OSX coding, so I'm not sure how much I can help...

I can try to look at the "scroll to fast and trigger a refresh", 'cause it's something I need to fix in my control too...

avaidyam commented Oct 7, 2012

@Sephiroth87 Thanks so much, either way! TwUI is fairly similar to UIKit, so if you'd ever want to try OS X programming, you know where to start. ;D

@galaxas0, I fixed the scrolling issue in my component, but since you changed the logic part quite a bit, I'm not sure if it's going to help ;)

avaidyam commented Oct 8, 2012

That's alright! I'll try to scrape what I can and make it work. Thanks!

@jwilling @Sephiroth87 @CodaFi I've been trying to get this fast scroll issue to fix, but I'm going nowhere... would anyone like to help?

If this were UIKit I'd suggest checking the following method to see if the content was actually released while dragging instead of a quick scroll:

- (void)scrollViewDidEndDragging:(UIScrollView *)scrollView willDecelerate:(BOOL)decelerate

For TUIScrollView, an option could be to override -scrollViewDidEndDragging:(TUIScrollView *)scrollView, and check the content offset to see if the content drag offset signifies an intentional drag.

@jwilling But that would mean setting ourselves as the delegate, which prevents the TUITableView delegate from being sent.

I think that comment gave rise to a smarter idea: add a simple property that, when decelerating, is YES, if fast, NO if slow... or something like that. I'll figure it out.

I don't think just the decelerating property is enough...
Don't this scrollView have a tracking/dragging property?

I can add in a tracking property, (internally there is), and check to see if it's actually being tracked as it's slid up, but then that would mean as the refresh control doesn't draw itself as it "falls back" into a ball.

avaidyam added some commits Nov 9, 2012

Merge remote-tracking branch 'upstream/master' into upstream-refresh-…
…control

Conflicts:
	ExampleProject/Example/ExampleView.m
	TwUI.xcodeproj/project.pbxproj
Merge remote-tracking branch 'upstream/master' into upstream-refresh-…
…control

Conflicts:
	TwUI.xcodeproj/project.pbxproj

CodaFi commented Nov 15, 2012

You're going to have to merge upstream and redo the example before I do any more code review.

@CodaFi Done!

Some problems still.

  • Hitting the top at a high rate of speed still refreshes the table view. It should not do this.
  • When the PTR area hides after showing, it hides the header as well. If the header exists, it should retract to the header showing as well.

@jwilling Haha, I was JUST working on the first one! And I noticed the second one just now too, and have a probable fix for that. :P

@jwilling @Sephiroth87 @CodaFi Here's what I've thought up using suggestions above. It doesn't work properly, though, but I'm not getting any results with debugging... any help?

I still stand by my opinion that you need to use the scrollViewDidEndDragging. An easy way to do this would be to add onto -endGestureWithEvent:, and set your flags there. All we need to keep track of is if the user has their fingers down on the trackpad when the pull view wants to be shown. If we know the user isn't touching the trackpad, then it's easy to ignore it.

Simple, and it'll work.

That's what the dragging property does. In fact, I just thought of this: why not add KVO for dragging and see when it changes? This way we can keep that logic independent of the drawing logic.

You won't be able to observe that. It's a readonly getter of a different property.

Which is why I say we need to make it KVO-compatible! :)

Well then, go forth and produce pull requests!

Let the pull requests rain from my godly hands like Zeus' thunderbolts! I'm on it!

CodaFi commented Dec 3, 2012

@galaxas0 Merge upstream, I think this one has a better chance of succeeding.

avaidyam commented Dec 4, 2012

@CodaFi I still can't figure out how to get this last hitch to work...

CodaFi commented Dec 4, 2012

@galaxas0 You said it yourself: KVO dragging and use it to conditionalize the showing of the refresh control.

jwilling commented Dec 4, 2012

On a related note, iOS 6 doesn't even show the refresh control when the scrolling speed is super fast.

avaidyam commented Dec 4, 2012

@CodaFi WAIT THAT'S BRILLIANT WHATS WRONG WITH ME T_T I'll go and fix that later tonight. /facedesk

avaidyam commented Dec 4, 2012

@jwilling That can be done too!

avaidyam commented Dec 7, 2012

@CodaFi @jwilling The scroll locking was fixed! As far as I could tell... please test? :) 🍰

CodaFi commented Dec 12, 2012

@galaxas0 When the view containing the refresh control is expanded or contracted, the activity indicator expands and contracts with it (and it's horrifyingly ugly when that happens). Perhaps a look into the autoresizingmasks would help.

@CodaFi This issue was noticed by @jwilling in another fork, and was fixed there: I'm also rewriting the UI element catalog app to become more versatile.

@galaxas0 What are you trying to say? If you're wanting this merged you need to fix it here as well, and merge from master as well.

@jwilling But didn't you tell me to pretty much rewrite the example app? If I'm going to do that, this seems quite trivial to fix. I'll merge from master.

@jwilling @CodaFi I've identified and fixed all issues, and merged upstream. 📬

Some issues remain:

  • When the scrolling bounce occurs, although the bug has been fixed where it refreshed, it still shows. On iOS if you get the bounce, it does not even show the refresh control.
  • Once the refresh is complete, it collapses to hide the header view. If the header view exists, it should keep it shown after refresh.

@jwilling I've fixed these issues (hopefully the last!) 📬

Now there is no collapsing animation.

The arrow is also messed up in multiple ways. Take a look:
Screen Shot 2012-12-21 at 12 28 05 AM

@jwilling oh great goblins >_> Alright, I'll get to fixing that.

jwilling commented Jan 1, 2013

I'll take a look at this once you fix the broken arrow drawing. 📫

@atomkirk atomkirk referenced this pull request in mdznr/What-s-New Jun 2, 2014

Merged

Fix typo in comment #5

mdiep commented Jul 17, 2014

We're killing TwUI (or at least our fork of it), so I'm going to close this.

@mdiep mdiep closed this Jul 17, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.