Handle no model argument being passed #1286
Conversation
|
Turns out the libraries |
|
Thank you, @erenoku! I don't have time to review right now, but I'll take a look soon. Tomorrow, if I can, or next week, if not. |
There was a problem hiding this comment.
Thank you, @erenoku! This is great!
I've taken a close look at the code and left some comments. In addition, there are a few more problems that need to be addressed before this is ready to merge:
- The CI build needs fixing, as you note. It looks like the CI file you linked doesn't need to install anything for XDG Desktop Portal, so the build might fix itself, if we use that instead (I've also left a comment on that topic).
- It's not up-to-date with the
mainbranch and needs a rebase. There are no conflicts, so I could do this automatically when merging, but I'd like to see how this looks together with Fix text of status messages looking jagged #1275, and if maybe a similar fix is required for the new UI element. - There are some compiler and Clippy warnings that need fixing. Those would fail the CI build, once the other CI issue is addressed.
- There's a panic when quitting the application:
I'm actually not sure if this is new, or if it was there before. In any case, it would be nice to get it fixed, but it's not critical.
thread '<unnamed>' panicked at 'Expected channel to never disconnect: RecvError', crates/fj-host/src/evaluator.rs:22:18 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
erenoku
force-pushed
the
handle_no_model
branch
from
d208d2c to
e564acb
Compare
|
Thanks for the feedback. |
| /// Event that are passed between the event_loop and gui | ||
| pub enum GuiEvent { | ||
| AskModel, | ||
| LoadModel(PathBuf), | ||
| } |
There was a problem hiding this comment.
Removing GuiEvent outright is perfectly fine, but just for the record, I think it would have been slightly nicer to keep a struct AskModel; and a struct LoadModel(PathBuf); around, to make those Sender/Receiver types below a bit more descriptive (Sender<AskModel> instead of Sender<()>, for example).
But this is only a minor thing! Just mentioning it for future reference.
| #[cfg(target_arch = "wasm32")] | ||
| todo!("Picking folders does not work on wasm32") |
There was a problem hiding this comment.
This (and the other cfgs) aren't great, but I can't think of a better way to do it right now. Maybe the file picking feature should move out of fj-viewer and into fj-window or fj-app? No idea. We'll figure it out at some point!
|
Hmm, looks like GitHub is being weird and sent my review comments before I actually wrote the main review itself. Please stand by, I'll finish that shortly. |
There was a problem hiding this comment.
Thank you for the changes, @erenoku!
I've left some comments, but all of those are about minor things. I'll merge this right now. Feel free to create a follow-up PR with additional changes, if you want, or don't 😄
I think all the issues are fixed now but I could not reproduce the panic when quitting the application. If it consistently happens on your machine could you upload the backtrace.
There is no backtrace, beyond the file/line from the panic message, as that panic happens in the root scope of one of the threads.
If you're not seeing it, don't worry. It's not critical anyway, and it'll be much easier for me to debug, since I can reproduce it consistently. I've made a note about that, and will take a look next week, mostly likely.
|
I fixed the panic I was seeing: #1296 |
This PR adds support for starting fj-app with no arguments.
If no model argument is passed or no
fj.tomlfile is present start the UI normally but display a warning and a button to open file picker in the middle of the screen. If the app is in export only mode displayno_model_errorand exit. The communication betweeneguiand the mainevent_loopis handled bycrossbeam_channelsand theGuistruct stores aGuiStateto store whether there is a model opened.