Skip to content

Commit

Permalink
Possible fix for moagrius#338.
Browse files Browse the repository at this point in the history
  • Loading branch information
@p-lr
p-lr committed Aug 21, 2016
1 parent 0624a3c commit ff17b91
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 6 deletions.
3 changes: 3 additions & 0 deletions demo/src/main/java/tileview/demo/RealMapTileViewActivity.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import com.qozix.tileview.TileView;
import com.qozix.tileview.markers.MarkerLayout;
import com.qozix.tileview.widgets.ZoomPanLayout;

import java.util.ArrayList;

Expand Down Expand Up @@ -88,6 +89,8 @@ public void onCreate( Bundle savedInstanceState ) {
// we're running from assets, should be fairly fast decodes, go ahead and render asap
tileView.setShouldRenderWhilePanning( true );

// allow zooming out to sse the entire map
tileView.setMinimumScaleMode(ZoomPanLayout.MinimumScaleMode.FIT);

This comment has been minimized.

Sign in to view

Copy link
@moagrius

moagrius Aug 21, 2016

i'd prefer this in another demo - i like the RealMapTileViewActivity to show the widget in it's best possible configuration
}

private MarkerLayout.MarkerTapListener markerTapListener = new MarkerLayout.MarkerTapListener() {
Expand Down
12 changes: 6 additions & 6 deletions tileview/src/main/java/com/qozix/tileview/widgets/ZoomPanLayout.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ public class ZoomPanLayout extends ViewGroup implements
private int mBaseHeight;
private int mScaledWidth;
private int mScaledHeight;
private int mTranslateX = 0;
private int mTranslateY = 0;

private float mScale = 1;

Expand Down Expand Up @@ -118,21 +120,18 @@ protected void onLayout( boolean changed, int l, int t, int r, int b ) {
final int width = getWidth();
final int height = getHeight();

int translateX = 0;
int translateY = 0;

if( mScaledWidth < width ) {
translateX = width / 2 - mScaledWidth / 2;
mTranslateX = width / 2 - mScaledWidth / 2;
}

if( mScaledHeight < height ) {
translateY = height / 2 - mScaledHeight / 2;
mTranslateY = height / 2 - mScaledHeight / 2;
}

for( int i = 0; i < getChildCount(); i++ ) {
View child = getChildAt( i );
if( child.getVisibility() != GONE ) {
child.layout( translateX, translateY, mScaledWidth + translateX, mScaledHeight + translateY );
child.layout( mTranslateX, mTranslateY, mScaledWidth + mTranslateX, mScaledHeight + mTranslateY );
}
}
calculateMinimumScaleToFit();
Expand Down Expand Up @@ -505,6 +504,7 @@ public boolean canScrollHorizontally( int direction ) {

@Override
public boolean onTouchEvent( MotionEvent event ) {
event.offsetLocation( -mTranslateX, -mTranslateY );

This comment has been minimized.

Sign in to view

Copy link
@moagrius

moagrius Aug 21, 2016

this makes me a little hesitant, since we're mutating that event incorrect, so if someone else really did want to have a TouchEvent listener (or override onTouch), i wouldn't want to give them back incorrect information (e.g., maybe they want to see how close the touch happened from the visible tiles).

what do you think of how he did it in moagrius#335?

This comment has been minimized.

Sign in to view

Copy link
@p-lr

p-lr Aug 21, 2016

Author Owner

I agree

This comment has been minimized.

Sign in to view

Copy link
@p-lr

p-lr Aug 21, 2016

Author Owner

I will check the proposal in #335

This comment has been minimized.

Sign in to view

Copy link
@moagrius

moagrius Aug 21, 2016

sounds good. i've got to step out for a bit but will be back later. if you find a fix you're 100% happy with go ahead and commit it (since this is kind of a hotfix) - don't worry about versions, etc - there are a number of commits in master that i need to include in the next release (which would be motivated by this fix). lmk if anything doesn't make sense.

thanks
boolean gestureIntercept = mGestureDetector.onTouchEvent( event );
boolean scaleIntercept = mScaleGestureDetector.onTouchEvent( event );
boolean touchIntercept = mTouchUpGestureDetector.onTouchEvent( event );
Expand Down

28 comments on commit ff17b91

@moagrius
Copy link

Choose a reason for hiding this comment

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

thanks for jumping on this peter. let me know if my comments make sense

@p-lr
Copy link
Owner Author

@p-lr p-lr commented on ff17b91 Aug 21, 2016

Choose a reason for hiding this comment

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

ok. I realize now this was not a good idea. We need to find another way.

@moagrius
Copy link

Choose a reason for hiding this comment

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

what about offsetting the events in those specific methods (e.g., MarkeryLayout.processHit, and I forget the other...) by getTop/getLeft?

@p-lr
Copy link
Owner Author

@p-lr p-lr commented on ff17b91 Aug 21, 2016

Choose a reason for hiding this comment

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

i tried it. It seems to work. But i did not make a complete check, to see if it doesn't break something else.

@moagrius
Copy link

Choose a reason for hiding this comment

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

it'll probably also need to be offset in HotSpotManager. processHit

@p-lr
Copy link
Owner Author

@p-lr p-lr commented on ff17b91 Aug 21, 2016

Choose a reason for hiding this comment

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

yes

@p-lr
Copy link
Owner Author

@p-lr p-lr commented on ff17b91 Aug 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Adding :

x -= getLeft();
y -= getTop();

in MarkerLayout.processHit() works. But HotSpotManager is not a ViewGroup so those methods don't exist.

@moagrius
Copy link

Choose a reason for hiding this comment

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

(thumb typing on phone, pls excuse lack of formatting and detail)

a. maybe give the hotspotmanager a reference to the tileview instance, the mTikeView.getX/Y in processHit

b. maybe offset setters in hotspotmanager, updated in tileview onLayout

@p-lr
Copy link
Owner Author

@p-lr p-lr commented on ff17b91 Aug 21, 2016

Choose a reason for hiding this comment

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

As for the HotSpotManager, i'm not quite sure how it works (no demo uses it, and btw the example in the readme uses an api than doesn't exists : TileView.addHotSpot( HotSpot, HotSpotTapListener )).

a. maybe give the hotspotmanager a reference to the tileview instance, the mTikeView.getX/Y in processHit

In fact, we need to use the mMarkerLayout offsets, not the mTileView ones.
As the two processHits are only called in TileView.onSingleTapConfirmed, it could be done like this :

@Override
  public boolean onSingleTapConfirmed( MotionEvent event ) {
    int x = (int) (getScrollX() + event.getX() - getX() - mMarkerLayout.getLeft() );
    int y = (int) (getScrollY() + event.getY() - getY() - mMarkerLayout.getTop() );
    mMarkerLayout.processHit( x, y );
    mHotSpotManager.processHit( x, y );
    return super.onSingleTapConfirmed( event );
  }

it works, but not elegant.

@p-lr
Copy link
Owner Author

@p-lr p-lr commented on ff17b91 Aug 21, 2016

Choose a reason for hiding this comment

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

I have to go now, i come back tomorrow.

@moagrius
Copy link

Choose a reason for hiding this comment

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

it works, but not elegant

I'm basically +1 on this but will post back later, or tomorrow, with details

I have to go now, i come back tomorrow

no problem, thanks peter

@krabo0om
Copy link

@krabo0om krabo0om commented on ff17b91 Aug 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I would like to point out that bug is just the result of another (kind of) bug:
The MarkerLayout wraps the image, thus cutting of the view of any marker which would be placed at the image's top and reaching into the whitespace. If the MarkerLayout fills the whole TileView than one could not just place a marker into the whitespace but this should also solve this bug.

@moagrius
Copy link

Choose a reason for hiding this comment

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

with the caveat that i wasn't really involved in this feature, IIUC having MarkerLayout fill all space would not be a fix. imagine a marker at 10% from top and 10% from left, on a 1000x1000 screen, and the tileview pinched to 100x100 and centered, which means the visible edge is at 950, so the marker should be at 960. if the markerlayout filled the screen, it'd be at 10 dots from top left, not 960.

@peterLaurence does that sound right?

@moagrius
Copy link

Choose a reason for hiding this comment

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

@Override
public boolean onSingleTapConfirmed( MotionEvent event ) {
  int x = (int) (getScrollX() + event.getX() - getX() - mMarkerLayout.getLeft() );
  int y = (int) (getScrollY() + event.getY() - getY() - mMarkerLayout.getTop() );
  mMarkerLayout.processHit( x, y );
  mHotSpotManager.processHit( x, y );
  return super.onSingleTapConfirmed( event );
}

Like i said, I'm basically +1 on this. instead of using mMarkerLayout.getLeft/Top(), i'd probably make the offsets calculated in ZoomPanLayout protected, and read those here instead.

does that make sense? sound reasonable?

@p-lr
Copy link
Owner Author

@p-lr p-lr commented on ff17b91 Aug 22, 2016

Choose a reason for hiding this comment

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

@moagrius just a sec, i am testing what @krabo0om said about MarkerLayout cutting some marker's view.

@p-lr
Copy link
Owner Author

@p-lr p-lr commented on ff17b91 Aug 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

with the caveat that i wasn't really involved in this feature, IIUC having MarkerLayout fill all space would not be a fix. imagine a marker at 10% from top and 10% from left, on a 1000x1000 screen, and the tileview pinched to 100x100 and centered, which means the visible edge is at 950, so the marker should be at 960. if the markerlayout filled the screen, it'd be at 10 dots from top left, not 960.

It makes sense, but sadly, i just tested with the fix you're +1 with a map with a marker at 10% from top and 10% from left. The marker's view was cut.

screenshot_20160822-215252

@p-lr
Copy link
Owner Author

@p-lr p-lr commented on ff17b91 Aug 22, 2016

Choose a reason for hiding this comment

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

So i am thinking, if the MarkerLayout fills the TileView, the logic of marker positioning would be completely changed, taking into account the eventual offsets top and/or left. It's feasible.
Basically, instead of offsetting the MarkerLayout, marker's positions take into account the offsets.

@moagrius
Copy link

Choose a reason for hiding this comment

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

yep, i think you're right. i think that's the only way forward, other than deprecating fill mode... i always felt like we got away with that too easily). we need to remember HotSpots too (maybe even paths?) but hopefully we can take whatever happens here and migrate the concept.

@p-lr
Copy link
Owner Author

@p-lr p-lr commented on ff17b91 Aug 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

btw, something is unclear to me about HotSpots. As the HotSpotManger is not a ViewGroup, when we offsets layouts in the case of FIT mode, the HotSpotManger is not affected, right?

@moagrius
Copy link

Choose a reason for hiding this comment

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

that's a good point, it just looks at scale... should be fine. I'll double check when markers are done. great catch peter

@p-lr
Copy link
Owner Author

@p-lr p-lr commented on ff17b91 Aug 22, 2016

Choose a reason for hiding this comment

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

ok, thanks confirming me this. I will do some experimentation with markers on FIT mode (try to implement the idea above).

@moagrius
Copy link

Choose a reason for hiding this comment

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

👍

@p-lr
Copy link
Owner Author

@p-lr p-lr commented on ff17b91 Aug 22, 2016

Choose a reason for hiding this comment

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

in fact, i just realized there is maybe a very simple solution. The problem is that marker's view are clipped?
By adding setClipChildren( false ); in ZoomPanLayout constructor, the issue is solved :
screenshot_20160822-225841

From the doc for setClipChildren :

By default, children are clipped to their bounds before drawing. This allows view groups to override this behavior for animations, etc.

@p-lr
Copy link
Owner Author

@p-lr p-lr commented on ff17b91 Aug 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

So, there is still the HotSpotManager thing to fix, because that component isn't aware of the eventual offsets. By that i mean that the getMatch method only looks at the scale.

@moagrius
Copy link

Choose a reason for hiding this comment

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

By adding setClipChildren( false ); in ZoomPanLayout constructor, the issue is solved :

awesome - could we do setClipChildren in MarkerLayout instead of ZoomPanLayout?

So, there is still the HotSpotManager thing to fix, because that component isn't aware of the eventual offsets. By that i mean that the getMatch method only looks at the scale.

yeah - do you want to do the hotspot thing or would you like me to jump in?

@p-lr
Copy link
Owner Author

@p-lr p-lr commented on ff17b91 Aug 22, 2016

Choose a reason for hiding this comment

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

awesome - could we do setClipChildren in MarkerLayout instead of ZoomPanLayout?

Tried it : it doesn't work. I think that in that case the ZoomPanLayout clips the MarkerLayout to its bounds, even if the later doesn't clips its children.

yeah - do you want to do the hotspot thing or would you like me to jump in?

Yes i think you're better placed to do it as it's more general api design.

@moagrius
Copy link

Choose a reason for hiding this comment

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

sounds good, i'll take it from here?

@p-lr
Copy link
Owner Author

@p-lr p-lr commented on ff17b91 Aug 22, 2016

Choose a reason for hiding this comment

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

ok

Please sign in to comment.