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

CLI Fixes & Tweaks #2846

Merged
merged 56 commits into from
Sep 13, 2024
Merged

CLI Fixes & Tweaks #2846

merged 56 commits into from
Sep 13, 2024

Conversation

DogeDark
Copy link
Member

@DogeDark DogeDark commented Aug 15, 2024

What's Included

  • Fixes scrolling on vscode (enabled mouse capture)
  • Adds scroll modifier, scroll at 5 lines per scroll while holding shift key.
  • Adds error handling for manganis failure that still lets the build run.
  • Revises TUI rendering code.
  • Move TUI "info bars" to the bottom of the terminal.
  • Removal of the "listening at x...". Pressing [o] open does the trick and the possibility to remove it was mentioned so I did so anyways (too lazy to make it responsive with the terminal size). Let me know if we do want to keep it.
  • Revised logging system with tracing
  • Working [c] clear keybind. This has been removed.
  • Removal of [h] hide keybind text (it does nothing)
  • Some opinionated cleanups and renames to make tui logic easier to understand.
  • Rolling log file & maybe add some more "internal" logging. Closes CLI Rolling Log File #2764
  • Removes log tabs. Closes CLI: Color-code logs and get rid of tabs #2857
  • Combines info bars.
  • Working and good text selection.
  • Print launch URL in console.
  • Handle log lines properly and add formatting.
  • Move MessageSource to a more reasonable location.
  • Add some background styling to powerline (info bar) - Tried this and it doesn't look the greatest.
  • Log Filtering
  • Final Cleaning & Changes - I could do this forever
  • Test Linux

Todo

  • Test Mac

If there's time... Decided these are out of the scope of this PR.

  • Only display certain logs like warnings once (show verbose option?). E.g. say "rebuilding" instead of everything and "rebuilding".
  • Proper structured logging [client <--> cli] with Dioxus Logger.
  • Some information in the [/] more keybind modal.

The New Logging

This PR introduces a new concept within the CLI: user-facing and internal logs.

User-facing logs are routed through the TUI into a single message buffer. Every user-facing log ends up as a Message with a MessageSource, verbosity Level, message content, and an automatically determined output_tab for the tab it will be shown in on the TUI.

Internal logs are always tracing-based. However, this doesn't mean any usage of tracing is an internal log. Tracing can be used to emit user-facing logs by providing the value dx_src as a tracing field. E.g. info!(dx_src = ?MessageSource::Build, "compilation finished"); Any usage of tracing with the dx_src, even if it isn't a MessageSource, will cause the log to be routed to the TUI. Any log without dx_src will be routed to the internal logs.

There is also an additional field dx_no_fmt which if set at all will cause the message content to be printed without any formatting for user-facing non-interactive-mode logs.

The current plan is for logs to be routed to a rolling log file which can be submitted with CLI bug issues for easier debugging. There are no current usages of internal logs.

Screenshots

Moved TUI info bars to the bottom of the screen:

image

@DogeDark DogeDark added enhancement New feature or request cli Related to the dioxus-cli program labels Aug 15, 2024
@ghost
Copy link

ghost commented Aug 17, 2024

I think tabs are a bad idea because the user would have to switch back and forth between their code terminal and their TUI terminal whereas if there were no tabs the user could just move the TUI terminal to a second monitor or whatever and never touch it (although as of right now I have to reload it manually to see any non-hot-reloadable changes, hopefully this will change in the future).

I suggest we follow what docker-compose does: there is just one tab and each "stream" of logs has a different color (in case of docker-compose each "stream" is the output of a particular container and in our case it could be the logs from the CLI itself, the logs from building target X (for fullstack projects we build for web and server targets simultaneously), etc. This can probably be implemented by having a tracing span with a field to differentiate "streams" and set appropriate color.

@ghost
Copy link

ghost commented Aug 17, 2024

i.e.

image

@DogeDark
Copy link
Member Author

@FragrantArmpit

I think tabs are a bad idea...

I've heard similar from a couple of other people. I don't plan to change this behavior in this PR but I do think it's something that should be discussed. If you could create a new issue about it, that would be great!

@DogeDark DogeDark marked this pull request as ready for review August 19, 2024 18:53
@Andrew15-5
Copy link
Contributor

Andrew15-5 commented Sep 6, 2024

When logs are already on "optimizing assets", the status message still shows the compiling that has been finished a long time ago:

screenshot

image

It should skip showing stages that already have been done. Logs and status message should be in sync.

No way... I retried a few times and in small window it updates the message almost immediately:

screenshot

image

But! When the terminal window is maximized, then the delay is always the same. Very strange how available TUI size changes the status message delay with logs.

@Andrew15-5
Copy link
Contributor

So, is the URL still present? If so, then maybe it's fine. But I want to see it. Does it compile now?

[dev]  INFO: Development server listening at http://127.0.0.1:8080

image

I guess this is how it looks now? Since we have a wrapper server starting at the very start, it's OK to send this message at the start.

@DogeDark
Copy link
Member Author

DogeDark commented Sep 6, 2024

@Andrew15-5 Thanks for the report about the overflow issue with arrow keys. I am looking into that now.

The other issue with the build messages is that it is building for fullstack and since two builds are running, they can conflict with the message output. What happened here is one build finished quicker and started optimizing but the status bar (compiling x%..) was showing the status for the slower build. I do not plan to fix this just yet as we wanna get this PR wrapped up.

And yes that is the new dev server message.

@DogeDark DogeDark marked this pull request as ready for review September 6, 2024 20:28
@Andrew15-5
Copy link
Contributor

Andrew15-5 commented Sep 6, 2024

I also would like this path (and others if they will appear) to be relative to the project root dir. This will make it smaller and more readable. Same for issue logs. So instead it will be [target/dx-dist-name].

image

@DogeDark
Copy link
Member Author

DogeDark commented Sep 6, 2024

@Andrew15-5 I think that would be best in a separate PR once this and the other big PR are merged. If you could post your request in the CLI megathread issue, that would be great.

@Andrew15-5
Copy link
Contributor

@jkelleyrtp
Copy link
Member

I ended up finding a fix to the vscode scrollback issue so we can actually select lines again.

The custom solution just wasn't working for me on macos and seemed too complex to keep in the CLI.

I also ended up moving the sections around for a slightly different appearance to be more "TUI-esque":

Screenshot 2024-09-13 at 1 12 13 AM

I think longer term we want to hide all warnings from cargo so the build log isn't so noisy, and then maybe add a keybind to toggle "verbose" mode.

@jkelleyrtp jkelleyrtp merged commit 8d68886 into DioxusLabs:main Sep 13, 2024
17 checks passed
@Andrew15-5
Copy link
Contributor

I think longer term we want to hide all warnings from cargo so the build log isn't so noisy, and then maybe add a keybind to toggle "verbose" mode.

Yeah, that's interesting. But it should mention somewhere that "All warnings are suppressed" if some warnings appear. It's a great alternative to the manual workaround: https://users.rust-lang.org/t/suppress-warnings-from-the-cargo-command/10536.

@DogeDark DogeDark deleted the cli-fixes branch September 14, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the dioxus-cli program enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: Color-code logs and get rid of tabs CLI Rolling Log File
3 participants