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 unit conversion feature in constants block and internal code frequency input #272

Merged

Conversation

BkPankaj
Copy link
Collaborator

Changes Made

  1. Implemented the addition of three units: k or K, m, M (representing Thousands, milli, Mega) to facilitate seamless conversion for both constant input and internal code block.
  2. Also, added the internal code block frequency section to accept units. If the value is a decimal and close to 0, it will be set to 0 (e.g., 4m).
  3. In the Cropper and various other blocks, multiple parameters are included within a single constant. To address this, each value is now separated by commas. Subsequently, the necessary conversions are applied, followed by the reintroduction of commas. This ensures the internal code functions smoothly without encountering any issues.

Related Issues

Fixes: #271

Screenshots

(Used all values for example purpose)

Canvas area - Cropper internal block with xywh of 5.73k,0.0012m,12M,248 and frequency 2.46M

data.json- with xywh of 5.73k,0.0012m,12M,248 (5730, 0.0000012, 12000000,248) and frequency 2.46M ( 2460000)

Canvas area - Threshold internal block with low of 5.0832K and high of 18m and frequency 3m

data.json- with low of 5.0832K (5083.2) and high of 18m (0.0180000) and frequency 3m i.e., 0.003 (0)

@toshan-luktuke
Copy link
Collaborator

Hi Pankaj, thank you for the contribution. The PR looks quite good... a question though

  1. At what point does the unit conversion take place in the Canvas? From my understanding it seems to be after the constant widget detects a change, if so does it change the value displayed in the Canvas or not?

In addition it is not desired to set low values equal to 0, as a user may purposefully use such low units in their program.

@BkPankaj
Copy link
Collaborator Author

Hello @toshan-luktuke ,
Yes, you are correct. Whenever it detects a change, it converts internally. I find this to be the best solution for real-time conversion. Basically, whenever the component called again (screen change) then it display converted value, the main aim is to avoid displaying the converted value on the spot. This is because it may make it difficult for the user to understand what happened. For example, if "27.98M" is displayed as "27980000" immediately, it could confuse the user. Therefore, after the screen changes, the converted value is shown. In other terms can say, If it is an internal block, and you visit it again internally, you will find the converted value. When you use it on the main canvas, it will show the value with the unit until the person enters the internal block, and then comes back to the main canvas. You will find the converted value then.

Regarding setting a low value to 0 for the internal code block, for that, I thought setting the frequency to 0.25 is close to 0, so I did that. No issue, I will change that to a decimal.

@BkPankaj
Copy link
Collaborator Author

@toshan-luktuke kindly check, I have changed parseInt to parseFloat for accepting decimal values inside the code block in the 'frequency' section.
Thank you for the suggestion.

@toshan-luktuke
Copy link
Collaborator

Alright, looks good, could you write comments for what the regex statements are doing once?
I think we can merge after that

@BkPankaj
Copy link
Collaborator Author

BkPankaj commented Feb 2, 2024

Hello @toshan-luktuke, could you please review my comments for the unit conversion function, handle change function, and regex statements?
Thank you!

@toshan-luktuke
Copy link
Collaborator

Do check my review once @BkPankaj

@BkPankaj
Copy link
Collaborator Author

BkPankaj commented Feb 2, 2024

@toshan-luktuke where you inserted review? Let me know which changes required. I have already committed changes as per you said in above comment.

@toshan-luktuke
Copy link
Collaborator

You can go up where I've highlighted the code, or look in the Files Changed tab and you should see it

@BkPankaj
Copy link
Collaborator Author

BkPankaj commented Feb 2, 2024

@toshan-luktuke I still cannot find your highlighted code or in the file changes. I don't know what I am missing. Could you please share a picture showing where it is or how to access it? It would be helpful for me as I have tried everything and am not able to find it.

@toshan-luktuke
Copy link
Collaborator

image
Its just this comment...
Alternatively, in the Files Changed Tab:
image

@BkPankaj
Copy link
Collaborator Author

BkPankaj commented Feb 3, 2024

Thank you for bringing it to my attention, @toshan-luktuke. I still couldn't locate your comment or review in the 'Files changed' section. Nevertheless, based on the images you provided, I acknowledge that I missed removing that line. Initially, I was considering functionality where, for example, if a user entered '10,000,' it would be handled. However, I later realized there might be several constants with more commas, so I abandoned that idea. Unfortunately, I forgot to delete that line. Additionally, in the comments commit, I only added a comment for that regex statement not seen use since the output was correct. In the new commit, I have rectified these issues. I have also added support for negative numbers, which was missing in the initial commits. Now, I believe there shouldn't be any issues.

}
}

return value.replace(/(\d)(?=(\d{3})+(?!\d))/g, '$1,');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please explain what this line is doing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

One more thing, I'm not sure what this line is doing exactly. It triggers only when there isn't a match, in which case its job is to add commas to a regular number. However, a normal number should not need those commas. In the examples you've uploaded I can only see tests for numbers with a suffix (k, M...). Could you show the working of your function with normal numbers?
Also do let me know if my understanding of the line is erroneous

@toshan-luktuke
Copy link
Collaborator

Looks good now, thanks a bunch for your contribution @BkPankaj !

@toshan-luktuke toshan-luktuke merged commit daf2e93 into JdeRobot:master Feb 4, 2024
@BkPankaj
Copy link
Collaborator Author

BkPankaj commented Feb 4, 2024

Thank you for merging @toshan-luktuke ! I'm excited to see this feature go live. Today marks my first contribution to a large codebase and project, and I'm very happy about it. If you have any feedback or suggestions, please feel free to let me know. Looking forward to more collaboration and making further contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants