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

Calculator: Add c constant #24576

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

skylar-eng
Copy link

@skylar-eng skylar-eng commented Jun 16, 2024

I have added the "c" constant to the calculator
This is the speed of light in a vacuum, 299,792,458 metres/second.
I Figured it would be an interesting thing to have added.

I also have created a quick and dirty c icon, that should be replaced.
It should be considered a placeholder for now.

I have added the "c" constant to the calculator, this is the speed of light in a vacuum, 299,792,458 metres/second.
Figured it would be an interesting thing to have.
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Jun 16, 2024
@BuggieBot
Copy link
Member

Hello!

One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

@skylar-eng
Copy link
Author

image

the WIP icon

Copy link
Member

@LucasChollet LucasChollet left a comment

Choose a reason for hiding this comment

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

Welcome to the project!

There are several issues that prevent the commit to be merged as is:

  • There are issues with your commit message, that's why the CI is failing. You can look at the details to see what's wrong.
  • You didn't add the image to the repo, so the code you submitted will make the Calculator crash on startup. That being said, I would rather not have an icon that having a "quick and dirty" one. The icon could be added later on.

To apply any of these, please fix up the modification in the current commit rather than adding a new one / creating a new PR. Don't hesitate to ask on Discord for help.

@@ -79,7 +79,9 @@ ErrorOr<int> serenity_main(Main::Arguments arguments)
constants_menu->add_action(GUI::Action::create("&Phi", TRY(Gfx::Bitmap::load_from_file("/res/icons/calculator/phi.png"sv)), [&](auto&) {
widget->set_typed_entry(Crypto::BigFraction { Crypto::SignedBigInteger(16180339887), power });
}));

Copy link
Member

Choose a reason for hiding this comment

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

I liked this empty line :/

@@ -79,7 +79,9 @@ ErrorOr<int> serenity_main(Main::Arguments arguments)
constants_menu->add_action(GUI::Action::create("&Phi", TRY(Gfx::Bitmap::load_from_file("/res/icons/calculator/phi.png"sv)), [&](auto&) {
widget->set_typed_entry(Crypto::BigFraction { Crypto::SignedBigInteger(16180339887), power });
}));

constants_menu->add_action(GUI::Action::create("&C", TRY(Gfx::Bitmap::load_from_file("/res/icons/calculator/c.png"sv)), [&](auto&) {
widget->set_typed_entry(Crypto::BigFraction { Crypto::SignedBigInteger(2997924580000000000), power });
Copy link
Member

Choose a reason for hiding this comment

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

You're doing extra work here, instead of manually multiplying by 10**10 and dividing by power, let's just not divide by anything (BigFraction has a constructor that default the denominator to zero):

Suggested change
widget->set_typed_entry(Crypto::BigFraction { Crypto::SignedBigInteger(2997924580000000000), power });
widget->set_typed_entry(Crypto::BigFraction { "299792458"_sbigint });

@skylar-eng
Copy link
Author

Welcome to the project!

There are several issues that prevent the commit to be merged as is:

  • There are issues with your commit message, that's why the CI is failing. You can look at the details to see what's wrong.

  • You didn't add the image to the repo, so the code you submitted will make the Calculator crash on startup. That being said, I would rather not have an icon that having a "quick and dirty" one. The icon could be added later on.

To apply any of these, please fix up the modification in the current commit rather than adding a new one / creating a new PR. Don't hesitate to ask on Discord for help.

Thank you! I will be sure to look and make these changes when I am back at my computer, in a couple of hours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 pr-needs-review PR needs review from a maintainer or community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants