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

Added buttons to select display type #5

Merged
merged 2 commits into from
Aug 4, 2017
Merged

Conversation

hiporox
Copy link
Contributor

@hiporox hiporox commented Jul 31, 2017

Added buttons to the bottom of the simulator screen that allows the user to choose how the values in registers are displayed (either hex, unsigned decimal, or decimal). This is very nice for debugging.

Currently does not support ASCII or changing the memory's display. These two features to come

Copy link
Owner

@kvakil kvakil left a comment

Choose a reason for hiding this comment

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

This is a really good change! I just added some comments about the code itself.

<div class="tile is-parent">
<article class="tile is-child">
<label>Display Settings</label>
<div class="tabs is-toggle">
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe this would be better stylistically as a bulma select?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this and I think it looks better as a toggle switch actually, but you are welcome to switch it to the select if you think it is better

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, let's keep it as a toggle switch for now.

@@ -155,4 +155,8 @@ import kotlin.browser.window
}
Renderer.updateRegister(id, sim.getReg(id))
}

@JsName("setDisplay") fun render_setDisplay(dis_type: String) {
Copy link
Owner

@kvakil kvakil Jul 31, 2017

Choose a reason for hiding this comment

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

Change this to camelCase? Also maybe a better name would be setRegisterDisplay.

@@ -166,7 +168,17 @@ internal object Renderer {
*/
fun updateRegister(id: Int, value: Int, setActive: Boolean = false) {
val register = getElement("reg-$id-val") as HTMLInputElement
register.value = toHex(value)
var shown_val: String
Copy link
Owner

Choose a reason for hiding this comment

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

Kotlin supports the when block, so this could be more cleanly written as:

register.value = when (displayType) {
    "hex" -> toHex(value)
    "dec" -> value.toString()
    "unsign" -> toUnsigned(value)
    else -> toHex(value)
}

private fun optSetVisibily(opt: String) {
var tabDisplay: HTMLElement?
for (option in opts) {
if (opt.equals(option)) {
Copy link
Owner

Choose a reason for hiding this comment

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

You don't need .equals for strings in Kotlin, just == is good. (If you need Object.equals from Java, that's ===.)

var tabDisplay: HTMLElement?
for (option in opts) {
if (opt.equals(option)) {
tabDisplay = document.getElementById("$option-opt") as HTMLElement
Copy link
Owner

Choose a reason for hiding this comment

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

Move this line out of the if statement?

for (option in opts) {
if (opt.equals(option)) {
tabDisplay = document.getElementById("$option-opt") as HTMLElement
tabDisplay.className = "is-active"
Copy link
Owner

Choose a reason for hiding this comment

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

We could make this a single line using the if statement:

tabDisplay.className = if (opt == option) "is-active" else ""

@hiporox
Copy link
Contributor Author

hiporox commented Aug 1, 2017

All great changes. Most of the caused by teaching myself Kotlin as I write it 😂
Will get these done as soon as I have time. Hopefully with the update that adds changes to memory and adds ASCII support

@hiporox
Copy link
Contributor Author

hiporox commented Aug 3, 2017

The ASCII renders a little weird, but that does make sense since most of ASCII is not human readable. Would like to add ASCII/Hex support at some point similar to MARS, but this should be good for now.

@kvakil
Copy link
Owner

kvakil commented Aug 3, 2017

Thanks for this! I'll take a look when I have the time.

@kvakil kvakil merged commit 3420d54 into kvakil:master Aug 4, 2017
@kvakil
Copy link
Owner

kvakil commented Aug 4, 2017

Okay, so I've made some changes:

  • I moved the tabs under the register & memory display and made it a bulma select. I may move it again once I add a settings panel.
  • I changed the handling of ASCII:
    • anything which is not ASCII (in [0, 255]) is displayed as hex -- this affects the register display
    • anything which is ASCII but not printable (not in [32, 127]) is printed with the unicode replacement character glyph
    • all ASCII characters are printed surrounded by '

I will probably reconsider some of this when I eventually update the memory display. Let me know if you have any suggestions!

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.

2 participants