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

Initial roi feature in segment app #410

Merged
merged 6 commits into from
Jun 30, 2020
Merged

Initial roi feature in segment app #410

merged 6 commits into from
Jun 30, 2020

Conversation

leoarc
Copy link
Contributor

@leoarc leoarc commented Jun 13, 2020

Description

  • Initial roi feature in the segment app .

  • Feature to extract roi from selected region

Other small changes :

  • Added progress percentage in snackbar

  • Typo fix

  • Use innerText instead of innerHTML

How Has This Been Tested?

OS : Ubuntu 20.04
Browser : Firefox

Types of changes

**What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

**Go over all the following points, and put an x in all the boxes that apply.
**(If you're unsure about any of these, don't hesitate to ask. We're here to help!)

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@leoarc
Copy link
Contributor Author

leoarc commented Jun 13, 2020

@birm any idea why are the tests not been triggered ?

@birm
Copy link
Member

birm commented Jun 13, 2020

Looks like it's here https://travis-ci.org/github/camicroscope/caMicroscope/builds/697929991
Not sure why it's not showing up.

@birm birm self-requested a review June 13, 2020 12:08
@@ -1178,13 +1253,13 @@ async function showInfo() {
store.get(name).onsuccess = function(e) {
inputShape = e.target.result.input_shape.slice(1, 3).join('x');
td = row.insertCell();
td.innerHTML = name.split('_').splice(1).join('_').slice(0, -3);
td.innerText = name.split('_').splice(1).join('_').slice(0, -3);
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for addressing these kinds of things!

@birm birm self-requested a review June 14, 2020 04:38
@birm
Copy link
Member

birm commented Jun 16, 2020

@Insiyaa I'm sorry I still can't add you as a reviewer directly.

@Insiyaa
Copy link
Contributor

Insiyaa commented Jun 16, 2020

Sorry for the delay, I'll review as soon as possible

Copy link
Member

@birm birm left a comment

Choose a reason for hiding this comment

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

A great start. Yes, a little very slow on big images, but that isn't your fault. 😄

@leoarc
Copy link
Contributor Author

leoarc commented Jun 20, 2020

@birm @Insiyaa I am adding to this since this is open . Hope that's ok ?

Copy link
Member

@birm birm left a comment

Choose a reason for hiding this comment

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

Minor typos, and the "Select Parameters" and results ("Details") screens are vulnerable to html injection

apps/model/model.js Outdated Show resolved Hide resolved
apps/segment/segment.js Outdated Show resolved Hide resolved
@Insiyaa
Copy link
Contributor

Insiyaa commented Jun 28, 2020

Looks good. Please check for spaces around the operators, rest it looks good.

@leoarc
Copy link
Contributor Author

leoarc commented Jun 28, 2020

@birm can you tell how to solve the html injection issue ? The input is coming from the db and not the user , so as long as the input is sanitized earlier it should be fine right ? Also as I need the html tags I can't use something like innerText .

@leoarc
Copy link
Contributor Author

leoarc commented Jun 28, 2020

@Insiyaa can you please elaborate on what you mean by spaces around operators ?

@birm
Copy link
Member

birm commented Jun 28, 2020

@leoarc the simplest solution would probably be to use a sanitize function to replace "<" and ">" with safe characters. See

function sanitize(string) {

Another option is to create each html node as is done here

let d1 = document.createElement('div');

@leoarc
Copy link
Contributor Author

leoarc commented Jun 28, 2020

Does this work ?
Screenshot from 2020-06-28 22-30-17

Screenshot from 2020-06-28 22-30-42

Screenshot from 2020-06-28 22-31-04

apps/model/model.js Outdated Show resolved Hide resolved
@leoarc
Copy link
Contributor Author

leoarc commented Jun 29, 2020

I may have missed some. Will do another pass in the next PR if that's ok .

@birm birm merged commit e2f9c18 into camicroscope:roi Jun 30, 2020
@leoarc
Copy link
Contributor Author

leoarc commented Jun 30, 2020

Thank you for the reviews ! And sorry for the constant bugging .

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.

3 participants