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

feat(ui): The status bar indicates that the panes are full screen and how many hidden panes are #450

Merged
merged 15 commits into from
Sep 27, 2021

Conversation

takezyou
Copy link
Contributor

@takezyou takezyou commented May 4, 2021

Implementation of #168
I plan to display it in the tab bar instead of the status bar

@hadronized
Copy link

Do you have a screenshot? Is there any ETA about the approval and merge of this PR?

@takezyou
Copy link
Contributor Author

takezyou commented Sep 5, 2021

@phaazon
This will be the one I just completed.
Screen Shot 2021-09-05 at 23 37 39

@takezyou takezyou changed the title wip: The status bar indicates that the panes are full screen and how many hidden panes are The status bar indicates that the panes are full screen and how many hidden panes are Sep 5, 2021
@takezyou takezyou marked this pull request as draft September 5, 2021 14:39
@takezyou takezyou marked this pull request as ready for review September 5, 2021 14:40
@takezyou takezyou changed the title The status bar indicates that the panes are full screen and how many hidden panes are feature: The status bar indicates that the panes are full screen and how many hidden panes are Sep 5, 2021
@takezyou takezyou changed the title feature: The status bar indicates that the panes are full screen and how many hidden panes are feat(ui): The status bar indicates that the panes are full screen and how many hidden panes are Sep 5, 2021
Copy link
Member

@imsnif imsnif left a comment

Choose a reason for hiding this comment

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

Hey, I'm really happy you took this up again! :)

I left some comments. The main one is that I think this should be in the status bar instead of the tab bar. Specifically instead of the line that starts with "Tip:" whenever we're in full screen.

Looking forward to merge this feature! I know for me personally it will be super helpful. I'm in full screen a lot and sometimes forget that I am, and then open new panes and everything gets messed up :)

default-plugins/tab-bar/src/tab.rs Outdated Show resolved Hide resolved
zellij-server/src/screen.rs Outdated Show resolved Hide resolved
@takezyou
Copy link
Contributor Author

takezyou commented Sep 9, 2021

@imsnif
I've made some corrections to the parts you reviewed.
However, I believe that the code and display of the UI part is not very good. Any advice would be appreciated!

Screen.Recording.2021-09-10.at.1.08.19.mov

@a-kenji
Copy link
Contributor

a-kenji commented Sep 10, 2021

Thanks @takezyou, this works already great.
There is still one more edge case that I saw while trying it out:

  • start zellij
  • open a pane
  • go fullscreen
  • open a pane

Now we are not in fullscreen anymore, but the status bar still shows
that we are.

@takezyou
Copy link
Contributor Author

@a-kenji
Thank you!
I've made the corrections to the parts you pointed out.
Also fixed the second line information to be accurate.

@imsnif
Copy link
Member

imsnif commented Sep 11, 2021

@takezyou - good progress!

I found another issue: it seems that the message only works if you're on the last opened tab.

To reproduce:

  1. Open a new pane
  2. Open a new tab
  3. Go back to the first tab
  4. Turn one of the panes fullscreen
  5. No message :(

About the visuals:
What do you think of adding some colors to make it more distinct? Maybe something like having it read "(FULLSCREEN): + 2 hidden panes" with the "FULLSCREEN" in orange and the number in green (with everything else in the normal color)?
Also notice to add a space after the plus :)

I'm going to be away for a few days, so I hope @a-kenji has time and can answer questions if you have them. Otherwise I can take another look on Wednesday.

@takezyou
Copy link
Contributor Author

@imsnif
Thanks for the confirmation!
I'll check regarding the issue.

I'll try to change the visuals as you suggested.
I'll ask @a-kenji when the fix is complete!

@takezyou
Copy link
Contributor Author

@a-kenji
I have fixed some of the points raised and would appreciate your review.

@a-kenji
Copy link
Contributor

a-kenji commented Sep 13, 2021

@takezyou
This looks much better, thank you for all the work.
I would like to show this information also in the locked mode,
after the -- Locked -- indicator, since some people spend more time in the
locked mode, than in other modes.

@takezyou
Copy link
Contributor Author

@a-kenji
I fixed it.
Added display of interface locked when in full screen. What do you think?
Screen Shot 2021-09-16 at 0 34 06

@a-kenji
Copy link
Contributor

a-kenji commented Sep 16, 2021

@takezyou
This is pretty good!
I would like a char to differentiate between the Locked and the
Fullscreen, probably a - but then I am in general happy with the feature.

@takezyou
Copy link
Contributor Author

@a-kenji
How about showing the original lock?
I feel like that would make it easier to distinguish between the two.

Screen Shot 2021-09-19 at 16 37 17

@a-kenji
Copy link
Contributor

a-kenji commented Sep 19, 2021

@takezyou
I agree, that is much better!
Thank you for the great work.
If you want you can squash some commits,
I will merge it later today.

@takezyou
Copy link
Contributor Author

@a-kenji
Thank you!
I'll go ahead and squash some commits now.

@takezyou
Copy link
Contributor Author

@a-kenji
All commits in one! Please confirm!

Copy link
Contributor

@a-kenji a-kenji left a comment

Choose a reason for hiding this comment

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

LGTM, I have tested this manually.
But the end to end tests fail all the time - do you maybe have an idea what this could be
@imsnif ?

Thanks for the work @takezyou

@imsnif
Copy link
Member

imsnif commented Sep 20, 2021

@a-kenji - from a brief glance it looks like there's a crash with a stacktrace in (all?) tests inside the container (this is distinct from the stacktrace of the failing test itself - you can see the tail end of it in the provided snapshot), eg.:
https://github.com/zellij-org/zellij/pull/450/checks?check_run_id=3654778354#step:12:2478

I would start by trying to replicate one test manually by SSHing into the container and doing all the steps that it does. You'll likely see the same stack trace and be able to understand what's causing it exactly.

I have a bit of a busy week, so not sure I'll have time to take a closer look soon unfortunately. If everyone is really stuck though I'll try to make time in one evening or the weekend.

@imsnif
Copy link
Member

imsnif commented Sep 20, 2021

Hum. Github doesn't seem to want to allow links to the build logs. Look for a line starting with "current_snapshot:" - this is a representation of what the test "sees on the screen", and in most cases there's the end of a stacktrace.

@a-kenji
Copy link
Contributor

a-kenji commented Sep 20, 2021

@imsnif
Thanks for the pointer!
This is what I see:

Timed out waiting for data on the SSH channel for test starts_with_one_terminal. Was waiting for step: Wait for app to load
cursor x: 2
cursor_y: 23
current_snapshot:

and then

thread 'main' panicked at 'Timed out waiting for test', src/tests/e2e/remote_runner.rs:390:25

I will try to recreate this in a virtual machine, but I suspect this will be somewhat slow.

@imsnif
Copy link
Member

imsnif commented Sep 20, 2021

No need for a virtual machine (unless I'm missing something?)

On this branch, do:

  1. cargo make build-e2e
  2. Start the docker environment, ssh into the container (I think the user/password are test/test, but it should be specified in the docker-compose file)
  3. Run zellij and do what the test does (eg. open a new pane)

@a-kenji
Copy link
Contributor

a-kenji commented Sep 21, 2021

@imsnif
Thanks!

No need for a virtual machine (unless I'm missing something?)

I had trouble getting docker compose working on my machine, but it seems
the bug I had has been fixed now. So I can run it natively now.
If I understand correctly the e2e-test builds the debug version, could that be the
reason for some of the timeouts? Especially the new tab one can take a long time in that
circumstance.

a-kenji and others added 7 commits September 21, 2021 16:39
Allow specifying only the tab name in the `tabs` section

- For example this is now possible:
```
tabs:
  - name: first
    parts:
      - direction: Vertical
      - direction: Vertical
  - name: second
  - name: third
```
  For that the tab section defaults the direction to
  `direction::Horizontal`

- Adds an error upon specifying a tab name inside the `parts` section
  of the tab-layout
…org#660)

* feat(plugins-manifest): Add a plugins manifest to allow for more configuration of plugins

* refactor(plugins-manifest): Better storage of plugin metadata in wasm_vm

* fix(plugins-manifest): Inherit permissions from run configuration

* refactor(plugins-manifest): Rename things for more clarity

- The Plugins/Plugin structs had "Config" appended to them to clarify
  that they're metadata about plugins, and not the plugins themselves.

- The PluginType::OncePerPane variant was renamed to be just
  PluginType::Pane, and the documentation clarified to explain what it
  is.

- The "service" nomenclature was completely removed in favor of
  "headless".

* refactor(plugins-manifest): Move security warning into start plugin

* refactor(plugins-manifest): Remove hack in favor of standard method

* refactor(plugins-manifest): Change display of plugin location

The only time that a plugin location is displayed in Zellij is the
border of the pane. Having `zellij:strider` display instead of just
`strider` was a little annoying, so we're stripping out the scheme
information from a locations display.

* refactor(plugins-manifest): Add a little more documentation

* fix(plugins-manifest): Formatting

Co-authored-by: Jesse Tuchsen <not@disclosing>
@imsnif
Copy link
Member

imsnif commented Sep 24, 2021

@a-kenji - as discussed offline, the e2e tests using the debug build was a very good find and I'm fixing it elsewhere. I don't think this is the issue here though. If you don't have time to take a look at this, I might be able to free some time up for this on the weekend. Let me know.

@a-kenji
Copy link
Contributor

a-kenji commented Sep 24, 2021

@imsnif
That is good to hear! Yes that is probably the plugin problem?
I might get time tomorrow to work on it - but I can not be sure yet.
If you get the time feel free to work on it. Maybe I will get to it and ping you!

@a-kenji
Copy link
Contributor

a-kenji commented Sep 24, 2021

@imsnif
I have gotten the tests running locally, they are super flaky.
I added the insta snapshot from the fullscreen change.

I think I found the issue for the flaky tests.
At least, if the test builds similar on gh to my local machine.
The release version runs fine. The musl release/debug builds sometimes
close panes on random for me. Can you check if that happens for you as well?

@imsnif
Copy link
Member

imsnif commented Sep 24, 2021

@a-kenji that's super weird. I just checked the musl release build and it runs just fine for me. What are you doing when it's closing panes for you?

I did a lot of work on the e2e tests today in the mirrored-sessions branch. It might be a pain to get them to work with this branch though (and I'm still not done - there's 1 test that often fails on the CI, but it specifically tests the mirrored sessions so you can safely ignore it).

@a-kenji
Copy link
Contributor

a-kenji commented Sep 24, 2021

@imsnif
I run cargo make build-e2e
And then run the musl build.
The release from our website is fine for me too.

I did a lot of work on the e2e tests today in the mirrored-sessions branch. It might be a pain to get them to work with this branch though (and I'm still not done - there's 1 test that often fails on the CI, but it specifically tests the mirrored sessions so you can safely ignore it).

Should I push the changes I made?
They are fairly minor.

@imsnif
Copy link
Member

imsnif commented Sep 25, 2021

@a-kenji - what changes?

@a-kenji
Copy link
Contributor

a-kenji commented Sep 25, 2021

I added the insta snapshot from the fullscreen change.

And I changed from debug to release build, but there are still some issues.
But maybe that could help us get to the problem quicker.

@imsnif
Copy link
Member

imsnif commented Sep 25, 2021

If it still passes once you do (all except the one test there, the mirror test, that isn't currently passing) then sure

* feat(sessions): mirrored sessions

* fix(tests): input units

* style(fmt): make rustfmt happy

* fix(tests): make mirrored sessions e2e test more robust

* refactor(sessions): remove force attach

* style(fmt): rustfmtify

* docs(changelog): update change

* fix(e2e): retry on all errors
@imsnif
Copy link
Member

imsnif commented Sep 27, 2021

@a-kenji - after merging my e2e stabilizations into main and then merging main here, the tests pass. Yay! :)

Merging this then. Thanks for your great work on this @takezyou ! And thanks for being patient with us in fixing our infrastructure issues before merging :)

@imsnif imsnif merged commit 4632e90 into zellij-org:main Sep 27, 2021
@takezyou
Copy link
Contributor Author

@imsnif
Thank you for merging with us too!
If I can find another good isuue, I'll try it.
If you have any recommendations, let me know!

@a-kenji
Thanks for all the advice!
I'm so glad this pull request got merged!

@a-kenji
Copy link
Contributor

a-kenji commented Sep 30, 2021

@takezyou
Thank you for keeping at it!
I am glad I could help.

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.

6 participants