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

Allow zenith to start when another instance is running, and add warnings #109

Merged
merged 3 commits into from
Oct 4, 2021

Conversation

alexmaco
Copy link
Contributor

related: #107

Right now, this only starts without writing to the db.

It adds a warning in the top right on the main screen, and an explanation on the help screen.
The wording and formatting of the warnings are subject to bikeshedding :).

I think the original issue could remain open, until a more complete handling for this scenario lands.

@bvaisvil
Copy link
Owner

Thank you very much for the PR. This is looking good, I'd like just a few changes though:

  • I'd like to change the message at the top from "Data not being recorded to database", to "History not recording, more Info: (h)".
  • Can we change the dimension of the help screen so that the message about the data record is more likely to be shown? on smaller dimensions it falls off the end.
  • Is it possible for the help screen to list why history isn't being recorded? Specifically I'm thinking here that if zenith was last run as root the database permissions won't be able to open by a normal user again. This will become more important with the iowait functionality I'm going to merge in requires root permission to use netlink.
  • A couple minor typos ('database' and 'started')

Thank you for your contributions and interest!

@alexmaco
Copy link
Contributor Author

* Can we change the dimension of the help screen so that the message about the data record is more likely to be shown? on smaller dimensions it falls off the end.

I've tried several layout changes, and cassowary (the layout engine in tui) seems to behave very weridly to say the least in some scenarios. To get it to consistently display on small terminals I ended up removing a lot of the margins.

Ultimately, I'd like to add scrolling and a scroll indicator, which will also be useful for the per-process view when the arguments don't fit on one screen (as is often the case for rustc).

@bvaisvil
Copy link
Owner

Removing the margins is fine by me. Let me know when I should give this another test!

@alexmaco
Copy link
Contributor Author

Well I did push the changes that remove the margin right before posting that comment, so this should be it, ready to test.
Are there any additional changes you think should be made?

@bvaisvil
Copy link
Owner

Okay, thanks for the updates. I very much appreciate your contributions! I tested the new commits and unfortunately found a few issues though that will need to be fixed:

  • The start up time seems unusually long (1-3 seconds). I think the lock check must be the issue, if I start with --disable-history it starts up very fast. So if this is just a matter of how long it takes to find if another zenith is running we should probably display a message to that effect (lock file found ... checking if another instance of zenith is still running)
  • There is an overflow bug when resizing the terminal
    thread '<unnamed>' panicked at 'attempt to subtract with overflow', src/render.rs:1365:21
  • The help doesn't display? or at least most of it is missing

Screen Shot 2021-09-22 at 1 36 30 PM

@bvaisvil
Copy link
Owner

Hiya, looks like you've addresses all of the issues I've identified. Were you planning any more changes or should I merge this in?

@alexmaco
Copy link
Contributor Author

I was planning on trying to see why there may be a long startup time, but for some reason it doesn't happen on my machine, so I can't measure what's taking it.

Other than that I had nothing more planned for this PR.

@alexmaco
Copy link
Contributor Author

@bvaisvil So in the end, if you can provide me with some precise measurement of exactly which lines take a long time at startup, I can probably fix it. But since I can't see the startup delay myself, I have nothing to add.

As a few other issues in the past, maybe this is also something that depends on OSX.

@bvaisvil
Copy link
Owner

bvaisvil commented Oct 4, 2021

Actually, I'm seeing the issue on linux (Fedora) but looks like it only consistently happens with the debug build. So I don't see any reason not to merge.

Thanks for the work on this!

@bvaisvil bvaisvil merged commit 629a4af into bvaisvil:master Oct 4, 2021
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