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

Add compiler information to dashboard (#1379) #1385

Merged
merged 1 commit into from Mar 24, 2019

Conversation

Projects
None yet
3 participants
@RomeuG
Copy link
Contributor

RomeuG commented Mar 24, 2019

This PR solves #1379.

Test plan (required)
With compiler:
image

Without compiler:
image

I assume the information is in the correct column of the dashboard, and since Compiled is empty, I assume Compiler can be empty too without being hidden.

@ITAYC0HEN

This comment has been minimized.

Copy link
Member

ITAYC0HEN commented Mar 24, 2019

Looks very nice!
Please write "N/A" when the information from the compiler is Not Available to you
Also, seems like the compiler is shown with priority to the right, the text should be shown from left to write, I'd prefer seeing the start of the sentence, instead of its end.

So just so it would be clearer, prefer seeing whatever is before "clang version..." and not the end

@RomeuG

This comment has been minimized.

Copy link
Contributor Author

RomeuG commented Mar 24, 2019

@ITAYC0HEN
Okay, will do! But it should be noted that the same does not happen to File. :-)

@ITAYC0HEN

This comment has been minimized.

Copy link
Member

ITAYC0HEN commented Mar 24, 2019

haha good catch! well, can you fill in the gaps and fix it for other places as well?
Fix both problems - the beginning of the sentence, and the "N/A"

@RomeuG

This comment has been minimized.

Copy link
Contributor Author

RomeuG commented Mar 24, 2019

@ITAYC0HEN should I do it on this issue or a separate one?

@ITAYC0HEN

This comment has been minimized.

Copy link
Member

ITAYC0HEN commented Mar 24, 2019

In this one is okay :)
Just add it to the description.
Thank you!

@RomeuG

This comment has been minimized.

Copy link
Contributor Author

RomeuG commented Mar 24, 2019

@@ -53,6 +53,7 @@ void Dashboard::updateContents()
this->ui->subsysEdit->setText(item2["subsys"].toString());
this->ui->endianEdit->setText(item2["endian"].toString());
this->ui->compiledEdit->setText(item2["compiled"].toString());
this->ui->compilerEdit->setText(item2["compiler"].toString());

This comment has been minimized.

Copy link
@xarkes

xarkes Mar 24, 2019

Member

This will overwrite what was written with the line just above.
If you believe the issue is just a key typo, then remove the line above.

This comment has been minimized.

Copy link
@xarkes

xarkes Mar 24, 2019

Member

You can also add a condition here as @ITAYC0HEN suggested, to check the value of item2["compiler"] and write N/A if empty.

@RomeuG

This comment has been minimized.

Copy link
Contributor Author

RomeuG commented Mar 24, 2019

@xarkes

This comment has been minimized.

Copy link
Member

xarkes commented Mar 24, 2019

Sorry I reviewed too quickly and thought you called setText on compilerEdit twice. My bad.

Maybe renaming compiledEdit variable to something more meaningful could be better to avoid any future misunderstanding. The field may contain something like:

    "compiled": "Sat Jan 16 18:19:00 2010",

So it's probably better to rename it to compilationDate or similar.

Also https://doc.qt.io/Qt-5/qtextedit.html#setText says that it will try to guess the text format and then call https://doc.qt.io/Qt-5/qtextedit.html#setPlainText if it's not HTML. Since we know we want to write plaintext, maybe it's a good time to switch all the setText calls to setPlainText :-)

Thanks for the PR :-)

@RomeuG

This comment has been minimized.

Copy link
Contributor Author

RomeuG commented Mar 24, 2019

@xarkes

This comment has been minimized.

Copy link
Member

xarkes commented Mar 24, 2019

Okay then I am merging this.

For your next PR I suggest that you add a private function and replace all calls to setText by that new function. That new function will take care of setting the field value which means:

  • Check that the given field is not empty, if it is, do edit->setPlainText("N/A")
  • If not, just call setPlainText with that value.
@xarkes

xarkes approved these changes Mar 24, 2019

@xarkes xarkes merged commit 86b0b56 into radareorg:master Mar 24, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.