-
-
Notifications
You must be signed in to change notification settings - Fork 110
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.
app/src/common/shared/com/igalia/wolvic/ui/adapters/Bookmark.java
Outdated
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/ui/adapters/NewTabAdapter.java
Outdated
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/ui/views/library/BookmarksView.java
Outdated
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/ui/widgets/settings/DisplayOptionsView.java
Outdated
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/ui/widgets/settings/DisplayOptionsView.java
Outdated
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/ui/widgets/WindowWidget.java
Outdated
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/ui/widgets/WindowWidget.java
Outdated
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/ui/widgets/settings/DisplayOptionsView.java
Outdated
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/ui/widgets/settings/DisplayOptionsView.java
Outdated
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/ui/widgets/settings/DisplayOptionsView.java
Outdated
Show resolved
Hide resolved
@felipeerias I have simplified the initial UI of the New Tab page like you suggested (the more complete UI is stored here as references for next PRs: https://github.com/haanhvu/wolvic/tree/issue1318-skeleton) I have also enabled going back to New Tab from another page or library panel. I'll wait for #1644 to be merged first to continue on it. Basically if #1644 works for library panel it would work for New Tab too |
b5206ba
to
4d02e46
Compare
00c5c51
to
b2934fc
Compare
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.
@felipeerias I have applied your suggestions and fixed the remaining bugs. Think we can undraft this now. Please review and see whether there are any other bugs. Thanks!
I'll add notes for some changes that don't seem very apparent:
app/src/common/shared/com/igalia/wolvic/ui/viewmodel/WindowViewModel.java
Outdated
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/ui/viewmodel/WindowViewModel.java
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/ui/widgets/WindowWidget.java
Outdated
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/ui/widgets/Windows.java
Outdated
Show resolved
Hide resolved
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 so much, this is a great job. I have a few comments pointing some things that need to be changed.
Additionally:
In TrayWidget.java
mCurrentContentTypeObserver
(line 608) we need to change this line:
if (contentType == Windows.ContentType.WEB_CONTENT) {
into this one:
if (contentType == Windows.ContentType.WEB_CONTENT || contentType == Windows.ContentType.NEW_TAB) {
otherwise there is a small glitch in the Bookmarks icon in the tray.
app/src/common/shared/com/igalia/wolvic/ui/widgets/Windows.java
Outdated
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/ui/viewmodel/WindowViewModel.java
Outdated
Show resolved
Hide resolved
private MediatorLiveData<ObservableBoolean> isNativeContentVisible; | ||
private MutableLiveData<ObservableBoolean> backToNewTabEnabled; |
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 see that you had to add new attributes in the WindowViewModel
to keep track of the current state, specially when the user browses back and forward, shows and hides the library, etc. Perhaps not all of these are needed and we can use the current URL to select the appropriate UI and content type (but maybe that is just wishful thinking on my part :-) ).
One question that comes to mind is whether these need to be part of the WindowViewModel
instead of being internal properties of WindowWidget
, for example. The view model is specially useful to listen to changes in particular values and to update XML layouts automatically.
Another observation is that this code tries to manipulate how the Back and Forward navigation works, and the result can be a bit confusing sometimes. This happens specially when we combine back/forward with the Home button.
In this case, I think it is better to leave that behaviour as something internal to the Web engine (Gecko or Chromium). Personally, I often use desktop Firefox as the reference when trying to understand how this navigation should work,
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.
Basically to handle navigation we need two things:
navigation_bar_navigation.xml
needs to know whether to enable or disable back/forward buttons.NavigationBarWidget.java
needs to know whether to go back/forward to web pages or new tab. Because the logics to handle these two are different.
That's why I needed to add new attributes in view model (I'll see if any of these can be moved elsewhere) and new logics to handle going back/forward to new tab.
For example, to handle going back we are currently using
wolvic/app/src/common/shared/com/igalia/wolvic/ui/widgets/NavigationBarWidget.java
Line 249 in 20abd0f
getSession().goBack(); |
This in turns calls
goBack()
from GeckoView, which only works for web pages:https://github.com/mozilla/gecko-dev/blob/3076c9156ef84aae253ffdc1d391e0bfab2c406b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java#L2388
That's why I added getSession().loadUri(UrlUtils.ABOUT_NEWTAB)
to handle going back to new tab in NavigationBarWidget.java
.
I study the codebase of desktop Firefox a bit. In desktop Firefox it seems like internal pages navigation is handled differently from webpages navigation too. Like in https://github.com/mozilla/gecko-dev/blob/master/browser/components/about/AboutRedirector.cpp
I understand your concerns to make this internal to web engines. But I checked the codebase of GeckoView as well and I haven't found anything to help navigating to internal pages (new tab in our case). Do you have any suggestions how to make this internal to web engines?
What's the status of this? |
The New Tab basically works. You can pick New Tab as homepage in Display settings, currently the page only has Wolvic logo (the more UI is stored here, Felipe and I agree to put it in later PRs: https://github.com/haanhvu/wolvic/tree/issue1318-skeleton), and the New Tab page can be navigated back/forward with library and other sites. Felipe raised some reviews mostly about how we handle navigation: #1574 (comment) I'm thinking about this and fixing other small reviews. |
…ic logo and add an option in Display Settings to set New Tab page as homepage This should be released when all the steps have been done and Igalia#1318 is closed.
First step for #1318: Create initial New Tab page that has Wolvic logo and add an option in Display Settings to set New Tab page as homepage
This should be released when all the steps have been done and #1318 is closed.