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

WIP: POC: bottom tabs (and "future of terminal UX" toughts) #15863

Closed

Conversation

razielanarki
Copy link

Summary of the Pull Request

Mocking up a switchable 'titlebarRow' and 'contentRow' in NonClientIslandWindow.cpp

...it was both easier than expected and works better than expected:

image

References and Relevant Issues

Detailed Description of the Pull Request / Additional comments

This is just a very-very crude POC, just to see if it's possible in the current implementation to place the tabs at the bottom, and mainly as an attempt to induce thought and spark discussion about ideas for streamlining UX around the _quakeMode window, particularly in relation to the settings panel (also considering the clutter of other terminal windows active at the same time, like elevated ones)

Checklists, Todos, etc., ... (as food for thought):

  • switching the order of the titlebar and the content pane is rather straightforward

  • window is draggable by the bottom titlebar, window controls work, tab drag and drop works between windows

    however, in it's current form it's far from being complete, both functionality/implementation and usability/design wise

  • (dependency(?) Question: Is winui also adopting the new Microsoft Edge TabBar style? microsoft-ui-xaml#7032

  • (bug) tab tear-off sometimes places the bottom-titlebar below visible bounds.

  • (to-do) setting UI for displaying the tabs on bottom for the Quake Mode window only (maybe for all terminal windows?)

  • (to-discuss) allow the Settings Pane to pop up/over the Quake Mode window (or All terminal windows)
    in it's own, possibly centered / default-sized window (in a _settingsMode window, so to speak).

    ...as currently the wide + narrow shape of the quake mode window hinders the usability of the settings panel embedded in a tab. (see window shapes in the above screenshot at 1920×1080, also considering keeping the current '1-column' design philosophy of the settings panel (which would lead to many of the options not being very visible at first glance, and would leave a lot of horizontal screen estate wasted)

  • (to-develop) unifying the elevated and non-elevated terminal processes into one window (using the shield icon per tab)
    for example, and if possible, by proxying elevated terminal I/O using named pipes for IPC with a non-elevated monarch?

Thanks for bearing with me, and kudos for an already immensely useful app!

contentRow in NonClientIslandWindow.cpp
@DHowett
Copy link
Member

DHowett commented Aug 22, 2023

WHOA

@KalleOlaviNiemitalo
Copy link

Does it work with separate title bar (presumably at the top) and tab bar (at the bottom)?

@zadjii-msft
Copy link
Member

Okay, there's a lot to unpack here. In general, we like to try to avoid giant threads that pile a bunch of unrelated topics into one - they tend to get highly unwieldly quick. I'm gonna try and reply to each of the bullets in your OP here, in order. In general though, aside from #835 we should probably move those discussions back out into linked threads.

  1. This PR is sweet. Yea it probably needs to be reconciled with "showTabsInTitlebar": false, but overall, I think it's cool.
  2. Dang that's neat. I concur though - we may want to iterate on what the design of this really is. Like, should the caption buttons (min/max/close) be in some thin bar at the top of the window (which remains draggable)?
  3. Yea obviously, Question: Is winui also adopting the new Microsoft Edge TabBar style? microsoft-ui-xaml#7032 is pretty important here. Or at least some way to style a TabView so that it has curvature on the top instead of the bottom. Alas, I'm suspecting that's impossible, because of Rectangular Tab border instead of circular #7213 (comment).
  4. Yea that should probably get fixed. There's probably code somewhere that does an offset and assumes that you're dropping relative to tabs on the top. I'd guess in AppHost::_handleMoveContent, to start.

As a brief intermission, I think it's okay to merge (some form of this) with the current appearance, under something like a "experimental.tabsPosition": "top"|"bottom" theme.window setting1. We mark it experimental, so folks are clear that it's... experimental. And prone to future breakages as we iterate on it.


These are the less-on-topic bits that should probably go to their own threads:

  1. This has two parts:
    • setting UI for displaying the tabs on bottom

      If we make it a theme setting, well, we don't really have a settings UI treatment for those currently. But putting it in the JSON is probably good enough for now.

    • for the Quake Mode window only

      This is Hard right now, and I wouldn't put that on you. We're basically using Add per-window-name global settings #9992 as "let different windows have different sets of settings" (including the quake window).

  2. This is sorta tracked in ... oh look at that, there isn't one. Shift+click on the Settings menu item just opens the settings.json, not settings in a new window. Add support for open-settings subcommand #5462 is maybe a needed pre-req. I'll file something for openSettings to open in a new window (and make sure to mention that we can probably ignore the original window's size).
  3. Alas, this is beyond any of us. See Megathread: sudo, runas, mixed elevation of tabs, etc. #1032, Scenario: Windows Terminal 2.0 Process Model Improvements #5000, Elevation Quality of Life Improvements.md. I'm not giving up on sudo (Sudo/command line only elevation? #146), but doing this at the Terminal level seems impossible / just wrong. I'd not want you to waste engineering time on this problem.

Back to the more on-topic bits: I'm gonna convert this to a draft and block it, based on the fact that we definitely need a setting for it. But I'm not opposed to merging this with the current design. Maybe as a canary-only feature (#15865).

Footnotes

  1. I could be convinced to make it a global setting rather than a per-theme one.

@zadjii-msft zadjii-msft marked this pull request as draft August 24, 2023 15:56
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

as mentioned - hook it up to a theme setting. You'll probably want to look at MTSMSettings.h, and AppHost::_updateTheme as reference points.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 24, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Aug 31, 2023
@microsoft-github-policy-service
Copy link
Contributor

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

@DHowett DHowett reopened this Sep 7, 2023
@DHowett DHowett removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. labels Sep 7, 2023
@zadjii-msft zadjii-msft added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 7, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Sep 14, 2023
@microsoft-github-policy-service
Copy link
Contributor

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants