Skip to content
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

Initial implementation of tabs bar #1468

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

felipeerias
Copy link
Collaborator

@felipeerias felipeerias commented Jul 8, 2024

The Tabs Bar is a small window that sits next to the current window and which lists the open tabs.

There are two implementations in this PR:

  • HorizontalTabsBar on the top of the window
  • VerticalTabsBar on the side of the window

The Tabs Bar is managed by VRBrowserActivity.

SessionStore now keeps a list of session change listeners so the tabs bar can be updated. SessionChangeListener and TabDelegate are moved to separate files.

The location of tabs can be changed in Settings -> Display.

WindowViewModel gets three new fields:

  • isTabsBarVisible

WidgetPlacement gets two new fields:

  • horizontalOffset
  • verticalOffset

These define a translation offset (in world dimensions), which is applied after the anchors have been calculated. The goal is to be able to move widgets from their initial position in a predictable way, to make space for the tabs bar when necessary.

These windows' offsets are updated in Windows.adjustWindowOffsets(). The primary use case is to make space for the horizontal tabs bar by moving the top widget, and to make space for the vertical tabs bar by moving the browser windows.

Note that Wolvic does not keep track of the relationship between windows and tabs. There is one single list of tabs that can be dynamically displayed on one or more windows. Unfortunately, this means that the interface tends to get a bit confusing when we have more than one window open.

@felipeerias felipeerias reopened this Sep 17, 2024
@felipeerias felipeerias force-pushed the felipeerias/tabs branch 2 times, most recently from 3756d48 to 8acbe88 Compare September 20, 2024 14:32
@felipeerias felipeerias force-pushed the felipeerias/tabs branch 2 times, most recently from e791cfd to 543e9bd Compare October 1, 2024 05:40
@felipeerias felipeerias marked this pull request as ready for review October 1, 2024 07:55
Copy link
Member

@svillar svillar left a comment

Choose a reason for hiding this comment

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

Haven't reviewed the code in detail yet but I have some comments

  1. Do we really want to have all those possibilities to show the tabs, I think we should select the best one and stick to it. Giving users too many options is not something I think is necessarily good
  2. The bar in horizontal mode is wider than the window
  3. The bar in horizontal mode hides the top bar widget which allows you to close windows
  4. The vertical mode does not look good in multiwindow mode IMO, it does overlap with multiple windows

Instead of having them permanently on top, what do you think about having a small UI element that when on hovered "unrolls" the tabs bar (either vertical or horizontal)?

@felipeerias
Copy link
Collaborator Author

Early video, to show the feature in action:

2024-10-02-tabs.mp4

@felipeerias
Copy link
Collaborator Author

I have updated the PR to hide the tabs bar when the window is being resized, and also to ensure that it updates its size correctly.

2024-10-03-tabs.mp4

@felipeerias felipeerias force-pushed the felipeerias/tabs branch 2 times, most recently from 1c3428e to 3fccc3e Compare October 15, 2024 07:05
@felipeerias
Copy link
Collaborator Author

I have fixed the interplay between the horizontal tabs bar and the TopBarWidget.

Regarding the vertical tabs bar, at the moment my idea is to simply move the unfocused window slightly to the left to make space for it. Something like this:

image

Copy link
Member

@svillar svillar left a comment

Choose a reason for hiding this comment

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

Nice job, it's actually quite some work. I left some comments inline.

Apart from that I have some comments about the UI:

  • Tabs are not well aligned with the windows, the vertical one is a little bit up, and the horizontal one is a little bit shifted right. It doesn't give a good impression
  • We should add some margin between the vertical tabs bar and the top bar widget (the one with the X to close the window) as they're too close.

@@ -358,6 +361,10 @@ public Session getActiveSession() {
return mActiveSession;
}

public List<Session> getSessions(boolean aPrivateMode) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this having the getSortedSessions already available?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

getSortedSessions() returns the last used sessions first, so the order changes all the time.

getSessions() keeps the list stable. New sessions are appended at the end of the list.

Eventually we can have a more complex algorithm, for example desktop browsers try to keep related tabs together.

@@ -43,7 +45,7 @@ public TrayViewModel(@NonNull Application application) {
isVisible.setValue(new ObservableBoolean(false));
time = new MutableLiveData<>();
pm = new MutableLiveData<>();
pm = new MutableLiveData<>();
needsTabsButton = new MutableLiveData<>(new ObservableBoolean(true));
Copy link
Member

Choose a reason for hiding this comment

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

I know this is a bit of bikeshedding, but perhaps we could have a more descriptive name, "needsTabs" does not mean anything to me. What about something like detachedTabsButton or similar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about tabsButtonInTray? It is simply a value to know if a tabs button should be included in the tray.

@@ -361,7 +361,7 @@ void updateScrollPosition(int offsetX, int offsetY) {
int verticalContentLength = mRecyclerView.computeVerticalScrollRange();
int verticalVisibleLength = mRecyclerViewHeight;
mNeedVerticalScrollbar = verticalContentLength - verticalVisibleLength > 0
&& mRecyclerViewHeight >= mScrollbarMinimumRange || mNeedHorizontalScrollbar;
&& mRecyclerViewHeight >= mScrollbarMinimumRange || mNeedVerticalScrollbar;
Copy link
Member

Choose a reason for hiding this comment

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

Good catch, this could even be a separate commit. Not sure if that was causing any actual visible bug.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to a separate commit.

@@ -101,7 +101,7 @@ public void setContextElement(WSession.ContentDelegate.ContextElement aContextEl
// Open link in a new tab
mItems.add(new MenuWidget.MenuItem(getContext().getString(R.string.context_menu_open_link_new_tab_1), 0, () -> {
if (!StringUtils.isEmpty(aContextElement.linkUri)) {
widgetManager.openNewTab(aContextElement.linkUri);
widgetManager.openNewTabForeground(aContextElement.linkUri);
Copy link
Member

Choose a reason for hiding this comment

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

Why this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the current architecture SessionStore simply passes events from each individual session to the tabs widget.

When a tab is opened in the background, its session does not call the onSessionAdded() event and therefore the tabs widget is not updated.

I have tried forcing a call to SessionStore.onSessionAdded() but then I get a crash in Gecko: "IllegalArgumentException: Tab with same ID already exists".

I am not sure but the problem might be that the background session is in an incomplete state until it is first shown.

@felipeerias
Copy link
Collaborator Author

My last pushed changes add two new fields to WidgetPlacement: horizontalOffset and verticalOffset.

These define a translation offset (in world dimensions), which is applied after the anchors have been calculated. The goal is to be able to move widgets from their initial position in a predictable way, for instance to make space for the tabs bar when necessary.

I am still writing the logic for that last step.

@felipeerias felipeerias force-pushed the felipeerias/tabs branch 2 times, most recently from 5a24a57 to a53ba7c Compare December 17, 2024 15:57
@felipeerias
Copy link
Collaborator Author

Updated videos:

ScreenRecording_2024.12.18-01.00.44.mp4
ScreenRecording_2024.12.18-01.02.36.mp4
output.mp4

@felipeerias felipeerias requested a review from svillar December 18, 2024 10:02
The Tabs Bar is a small window that sits next to the current window
and which lists the open tabs.

There are two implementations:
  - HorizontalTabsBar on the top of the window
  - VerticalTabsBar on the side of the window

The Tabs Bar is managed by VRBrowserActivity.

SessionStore now keeps a list of session change listeners.

TabDelegate is moved to a separate interface.

The current tabs style can be changed in Settings -> Display.

WindowViewModel gets a new field:
- isTabsBarVisible

Note that we don't (yet) link windows and tabs, so the interface
gets a bit confusing when we have more than one window open.

WidgetPlacement gets two new fields:
- horizontalOffset
- verticalOffset

These define a translation offset (in world dimensions), which is
applied after the anchors have been calculated. The goal is to be able
to move widgets from their initial position in a predictable way, to
make space for the tabs bar when necessary.

The windows' offsets are updated in Windows.adjustWindowOffsets().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants