Skip to content
This repository was archived by the owner on Feb 9, 2023. It is now read-only.

Fix NPE resulting from HoverViewState's callback after it already lost control #3

Merged
merged 4 commits into from
Dec 3, 2018
Merged

Fix NPE resulting from HoverViewState's callback after it already lost control #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fd052fa
Fix NPE - State 가 변한 후에 받는 콜백에서 HoverView 를 찾을 수 없어서 발생하던 에러 방지
SeoJungHong Nov 16, 2018
60db3fb
Fix NPE - HoverViewStateClosed 에도 적용
SeoJungHong Nov 16, 2018
7ccb2f1
fix IDE warning about logging tag too long
SeoJungHong Nov 16, 2018
8a56dcc
review - remove log
SeoJungHong Nov 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion hover/src/main/java/io/mattcarroll/hover/HoverViewStateClosed.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,19 @@
*/
class HoverViewStateClosed extends BaseHoverViewState {

private static final String TAG = "HoverMenuViewStateClosed";
private static final String TAG = "HoverViewStateClosed";

private HoverView mHoverView;
private boolean mHasControl = false;

@Override
public void takeControl(@NonNull HoverView hoverView) {
Log.d(TAG, "Taking control.");
super.takeControl(hoverView);
if (mHasControl) {
throw new RuntimeException("Cannot take control of a FloatingTab when we already control one.");
}
mHasControl = true;
mHoverView = hoverView;
mHoverView.notifyListenersClosing();
mHoverView.mState = this;
Expand All @@ -47,6 +52,9 @@ public void takeControl(@NonNull HoverView hoverView) {
selectedTab.disappear(new Runnable() {
@Override
public void run() {
if (!mHasControl) {
return;
}
mHoverView.mScreen.destroyChainedTab(selectedTab);
mHoverView.notifyListenersClosed();
}
Expand All @@ -59,6 +67,10 @@ public void run() {
}

private void changeState(@NonNull HoverViewState nextState) {
if (!mHasControl) {
throw new RuntimeException("Cannot give control to another HoverMenuController when we don't have the HoverTab.");
}
mHasControl = false;
Copy link

Choose a reason for hiding this comment

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

mHoverView 가 null 인지 체크하면 문제가 생길까요? PR 상으로는 mHoverView 의 존재 여부가 mHasControl 의 값하고 일치하는 것 같아서요ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

네 말씀하신대로 현재 mHoverView 존재 여부가 mHasControl 과 일치합니다. 하지만 이렇게 따로 만든건 hover 쪽에 PR을 날리기 위해서 이전 로직을 최대한 활용해서 NPE 만 해결하기 위함이었어요. 별건 아니지만 조금 더 Descriptive 한것 같기도 하네요..

Copy link

Choose a reason for hiding this comment

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

그렇군요ㅎㅎ 공감되긴 합니다만 아직 저는 추후 개발하면서 mHasControl 과 mHoverView 의 값이 consistency 가 깨질 위험이 있어 보여서, 실제 문제가 되는 mHoverView 값이 null 임을 확인하는게 더 직관적인 것 같은 생각이 들긴 하네요.

그래도 이 부분 판단은 헤일리 결정에 따르겠습니다 :)

Copy link
Author

Choose a reason for hiding this comment

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

@ssowonny 네! 아직은 로직이 복잡하지 않으니 일단 이대로 NPE 만 수정하고, 추후 mHasControl 관련 로직이 복잡해 진다면 실제로 문제가 되는 HoverView 를 체크하는 방향으로 전반적으로 수정하도록 하겠습니다.

mHoverView.setState(nextState);
mHoverView = null;
}
Expand Down
8 changes: 7 additions & 1 deletion hover/src/main/java/io/mattcarroll/hover/HoverViewStateCollapsed.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
*/
class HoverViewStateCollapsed extends BaseHoverViewState {

private static final String TAG = "HoverMenuViewStateCollapsed";
private static final String TAG = "HoverViewStateCollapsed";

private HoverView mHoverView;
private FloatingTab mFloatingTab;
Expand Down Expand Up @@ -108,6 +108,9 @@ public void takeControl(@NonNull HoverView hoverView) {
mHoverView.post(new Runnable() {
@Override
public void run() {
if (!mHasControl) {
return;
}
if (wasFloatingTabVisible) {
sendToDock();
} else {
Expand Down Expand Up @@ -303,6 +306,9 @@ private void sendToDock() {
mFloatingTab.dock(new Runnable() {
@Override
public void run() {
if (!mHasControl) {
return;
}
onDocked();
}
});
Expand Down
5 changes: 4 additions & 1 deletion hover/src/main/java/io/mattcarroll/hover/HoverViewStateExpanded.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
*/
class HoverViewStateExpanded extends BaseHoverViewState {

private static final String TAG = "HoverMenuViewStateExpanded";
private static final String TAG = "HoverViewStateExpanded";
private static final int ANCHOR_TAB_X_OFFSET_IN_PX = 100;
private static final int ANCHOR_TAB_Y_OFFSET_IN_PX = 100;
private static final int TAB_SPACING_IN_PX = 200;
Expand All @@ -57,6 +57,9 @@ class HoverViewStateExpanded extends BaseHoverViewState {
private final Runnable mShowTabsRunnable = new Runnable() {
@Override
public void run() {
if (!mHasControl) {
return;
}
mHoverView.mScreen.getShadeView().show();
mHoverView.mScreen.getContentDisplay().selectedTabIs(mSelectedTab);

Expand Down