-
-
Notifications
You must be signed in to change notification settings - Fork 106
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
Create New Tab page and add an option to set New Tab page as homepage #1574
base: main
Are you sure you want to change the base?
Conversation
@felipeerias @svillar (and other maintainers) This is a very draft initial implementation. Currently I just added a Homepage option that shows Bookmarks panel like this: https://drive.google.com/file/d/1feSPqru-LJJOmQ3kwPKKuEXal9dirkbr/view?usp=sharing I'm thinking of creating a specific New Tab page that shows bookmarks (and maybe other things too?). Do you have any ideas on how that New Tab page should look like? Is there any existing layout that I can use for this page? |
I think we should be consistent with what others do. See Desktop Firefox, Chrome, or Edge for inspiration |
64fb0d9
to
218b7b8
Compare
bf3f824
to
8ffd64b
Compare
01ac05b
to
96768a2
Compare
@felipeerias @svillar (and other maintainers) There's bug in the New Tab page that I'm not having any clues to debug. Basically, the hovering point is not exactly as it's showing. For example: The reverse is true too: when I actually hover in the item, the page doesn't recognize that and doesn't show the trash icon: Could you help me to debug this? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this is great work 🙂
In addition to these suggestions, we also need to add the New Page to the WindowViewModel
so other elements in the UI are updated correctly.
See WindowViewModel.setIsLibraryVisible()
and WindowViewModel.getIsLibraryVisible()
for reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the changes in this file are not needed for now. I would rather focus this PR on just getting the basic functionality. Let's implement the New Tab content in subsequent PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea but let's move it to a separate PR once we land this one.
@@ -111,6 +111,7 @@ public void updateUI() { | |||
mBinding.setBookmarksViewModel(mViewModel); | |||
mBinding.setCallback(mBookmarksCallback); | |||
mBookmarkAdapter = new BookmarkAdapter(mBookmarkItemCallback, getContext()); | |||
mBookmarkAdapter = new BookmarkAdapter(mBookmarkItemCallback, getContext()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated line.
SettingsStore.getInstance(getContext()).setHomepage(mDefaultHomepageUrl); | ||
} else if (checkedId == 1) { | ||
mBinding.homepageEdit.setVisibility(View.GONE); | ||
SettingsStore.getInstance(getContext()).setHomepage("about://newtab"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use UrlUtils.isNewTabUrl()
private int getHomepageId(String homepage) { | ||
if (homepage == getContext().getString(R.string.HOMEPAGE_URL)) { | ||
return 0; | ||
} else if (homepage == "about://newtab") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use UrlUtils.isNewTabUrl()
if (mNewTab != null) { | ||
setView(mNewTab, true); | ||
mViewModel.setIsFindInPage(false); | ||
final Runnable firstDrawCallback = mFirstDrawCallback; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, but I think that we need to have the same check as in showPanel()
below.
if (mRestoreFirstPaint == null && !isFirstPaintReady() && (mFirstDrawCallback != null) && (mSurface != null)) {
...
if(UrlUtils.isBookmarksUrl(uri.toString())) { | ||
showPanel(Windows.BOOKMARKS); | ||
if(UrlUtils.isBookmarksUrl(uri.toString())) { | ||
hideNewTab(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can make showPanel(…)
hide the New Tab view?
SettingsStore.getInstance(getContext()).setHomepage(mDefaultHomepageUrl); | ||
} else if (checkedId == 1) { | ||
mBinding.homepageEdit.setVisibility(View.GONE); | ||
SettingsStore.getInstance(getContext()).setHomepage("about://newtab"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use UrlUtils.ABOUT_NEWTAB
.
} | ||
|
||
private int getHomepageId(String homepage) { | ||
if (homepage == getContext().getString(R.string.HOMEPAGE_URL)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to use equals()
in Java to compare strings.
private int getHomepageId(String homepage) { | ||
if (homepage == getContext().getString(R.string.HOMEPAGE_URL)) { | ||
return 0; | ||
} else if (homepage == "about://newtab") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use equals()
and UrlUtils.ABOUT_NEWTAB
.
fix #1318