-
Notifications
You must be signed in to change notification settings - Fork 8
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
UI Remake #73
UI Remake #73
Conversation
f962697
to
efcd87f
Compare
I do |
Thanks. And I am not even half done :) |
7988727
to
1934703
Compare
BookmarksWinit stuff(Basically updated to 0.30 which requires a struct for the application instead of an event loop closure) Vulkano stuff(Updated to 0.34)
Settings/Config
Audio/Synthesizer
GUI Highlights
MIDI
Misc
Other changes not listed here are either long egui code or changes in other files to reflect any method changes mentioned above) |
@@ -164,12 +296,21 @@ impl GuiWasabiWindow { | |||
} | |||
}); | |||
|
|||
// If song is finished, pause |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need its own block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
Should I move it elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the extra set of {}
there's a couple like that in this file, not sure if that's necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I added those because some variables were only used there. For this case it won't make any difference so I can remove it if you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah only use that if it makes a difference for the logic imo, more variables in the scope isn't an issue
@arduano Made most of the changes requested. I'll wait for clarifications on the other comments before I fix the rest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the {} scope thing, looks ok
still some things to look at after this PR is merged though
@arduano Will fix that and merge. I also created issues about the rest we discussed that needs to be fixed and added everything in a milestone for 1.0.0. Let me know if this is ok or if there is something else we need to look at. |
Nothing off the top of my head |
You are going to hate me for this