Initial implementation of tabs bar #1468

felipeerias · 2024-07-08T14:47:52Z

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.

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.

svillar

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

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
The bar in horizontal mode is wider than the window
The bar in horizontal mode hides the top bar widget which allows you to close windows
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 · 2024-10-02T10:06:40Z

Early video, to show the feature in action:

2024-10-02-tabs.mp4

felipeerias · 2024-10-03T06:19:01Z

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 · 2024-10-15T07:37:43Z

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:

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 three new fields: - isTabsBarVisible - usesHorizontalTabsBar - usesVerticalTabsBar TopBarWidget uses two of these fields to update its placement so as to not overlap with the tabs bar. 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.

svillar

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.

svillar · 2024-11-22T09:15:38Z

app/src/common/shared/com/igalia/wolvic/VRBrowserActivity.java

        if (aPrevWindow != null) {
            updateWidget(mNavigationBar);
+            updateWidget(mTabsBar);


This is a mistake. You have the good one bellow with the null check

svillar · 2024-11-22T09:16:03Z

app/src/common/shared/com/igalia/wolvic/VRBrowserActivity.java

+                recenterUIYaw(WidgetManagerDelegate.YAW_TARGET_ALL);
+        } else if (Objects.equals(key, getString(R.string.settings_key_tabs_location))) {
+
+            Log.e(LOGTAG, "update tabs location");


Please remove this

svillar · 2024-11-22T09:16:11Z

app/src/common/shared/com/igalia/wolvic/VRBrowserActivity.java

+
+            // remove the previous widget
+            if (mTabsBar != null) {
+                Log.e(LOGTAG, "remove previous widget");


svillar · 2024-11-22T09:16:44Z

app/src/common/shared/com/igalia/wolvic/VRBrowserActivity.java

+                mTabsBar.releaseWidget();
+            }
+
+            if (mSettings.getTabsLocation() == SettingsStore.TABS_LOCATION_TRAY) {


I think a switch-case statement fits better for this use case

svillar · 2024-11-22T09:18:07Z

app/src/common/shared/com/igalia/wolvic/browser/engine/SessionStore.java

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

+    public List<Session> getSessions(boolean aPrivateMode) {


Do we really need this having the getSortedSessions already available?

svillar · 2024-11-22T09:21:04Z

app/src/common/shared/com/igalia/wolvic/ui/adapters/TabsBarAdapter.java

+        if (position == 0) {
+            return 0;
+        } else {
+            return mTabs.get(position - 1).getId().hashCode();


return position == 0 ? 0 : mTabs.get....

svillar · 2024-11-22T09:22:32Z

app/src/common/shared/com/igalia/wolvic/ui/viewmodel/TrayViewModel.java

@@ -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));


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?

svillar · 2024-11-22T09:23:55Z

app/src/common/shared/com/igalia/wolvic/ui/views/CustomFastScroller.java

@@ -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;


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

svillar · 2024-11-22T09:25:48Z

app/src/common/shared/com/igalia/wolvic/ui/widgets/HorizontalTabsBar.java

+import com.igalia.wolvic.browser.engine.SessionStore;
+import com.igalia.wolvic.ui.adapters.TabsBarAdapter;
+
+public class HorizontalTabsBar extends AbstractTabsBar implements SessionChangeListener {


I don't think the implements is needed, isn't it?

svillar · 2024-11-22T09:29:32Z

app/src/common/shared/com/igalia/wolvic/ui/widgets/menus/ContextMenuWidget.java

@@ -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);


felipeerias force-pushed the felipeerias/tabs branch from 641b268 to eb31be6 Compare September 13, 2024 07:11

felipeerias closed this Sep 13, 2024

felipeerias force-pushed the felipeerias/tabs branch from eb31be6 to 22da6fa Compare September 13, 2024 14:50

felipeerias reopened this Sep 17, 2024

felipeerias force-pushed the felipeerias/tabs branch 2 times, most recently from 3756d48 to 8acbe88 Compare September 20, 2024 14:32

felipeerias force-pushed the felipeerias/tabs branch 2 times, most recently from e791cfd to 543e9bd Compare October 1, 2024 05:40

felipeerias requested review from svillar and javifernandez October 1, 2024 07:55

felipeerias marked this pull request as ready for review October 1, 2024 07:55

svillar reviewed Oct 1, 2024

View reviewed changes

felipeerias force-pushed the felipeerias/tabs branch from 543e9bd to af15e21 Compare October 3, 2024 06:17

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

felipeerias force-pushed the felipeerias/tabs branch from 3fccc3e to 49e6435 Compare November 15, 2024 13:33

felipeerias requested a review from svillar November 19, 2024 15:21

svillar requested changes Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of tabs bar #1468

Initial implementation of tabs bar #1468

felipeerias commented Jul 8, 2024 •

edited

Loading

svillar left a comment

felipeerias commented Oct 2, 2024

felipeerias commented Oct 3, 2024

felipeerias commented Oct 15, 2024

svillar left a comment

svillar Nov 22, 2024

svillar Nov 22, 2024

svillar Nov 22, 2024

svillar Nov 22, 2024

svillar Nov 22, 2024

svillar Nov 22, 2024

svillar Nov 22, 2024

svillar Nov 22, 2024

svillar Nov 22, 2024

svillar Nov 22, 2024

Initial implementation of tabs bar #1468

Are you sure you want to change the base?

Initial implementation of tabs bar #1468

Conversation

felipeerias commented Jul 8, 2024 • edited Loading

svillar left a comment

Choose a reason for hiding this comment

felipeerias commented Oct 2, 2024

felipeerias commented Oct 3, 2024

felipeerias commented Oct 15, 2024

svillar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipeerias commented Jul 8, 2024 •

edited

Loading