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

Fix compiler warnings #10

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Andrea-moth
Copy link

Might need a quick review, I've added unwraps to functions that return results, but that might not be the best idea.

Other than that, should be good

// about the app to say other wise
//
// Feel free to replace
self.inner.send_event(event).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

I'd not recommand unwrap here, because it's not an issue that should panic.

In the long term, I think it would be a better idea to send the Result to the application layer and handle them there.

let mut content_size = Vec2::default();
let button_padding = ui.spacing().button_padding;

if self.editable {
Copy link
Owner

Choose a reason for hiding this comment

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

Seems weird using editable in L51, but uses self.editable again here. Same L72, L82, L98, L114.

// about the app to say other wise
//
// Feel free to replace
select_system_locales().unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

Recommand not to unwrap here. You can just ignore it or simply log the error (better).

// about the app to say other wise
//
// Feel free to replace
select_locales(&[language.id]).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

Same here. I18n issues should not panic the whole program.

.on_hover_text(fl!("control_capture"))
.clicked()
{
if state.can_capture {
Copy link
Owner

Choose a reason for hiding this comment

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

Screen capturing is not triggered by play button. The capture button should not be removed if there is not specific reasons.

@fralonra
Copy link
Owner

fralonra commented Jan 9, 2024

Thanks! I've done a quick review.

Apart from that, it's recommanded to use git mv to rename files rather than creating a new one then delete the old one, because it makes git to track in-file changes. You can make your changes in the orginal src/ui.rs and src/ui/highlight.rs and then git mv these files.

And better to re-organise commits (Currently there are two commits with the same description). Try git rebase -i.

BTW, are you using analyst tools other than rust-analyzer or you have a specific config? My vscode and rust-analyzer combination did not give me warnings like quotes, mut and &.

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.

None yet

2 participants