Tweak Mode: Some numbers ignored in second tab. #4017

Closed
erniejunior opened this Issue Oct 18, 2015 · 5 comments

Comments

Projects
None yet
3 participants
@erniejunior
Contributor

erniejunior commented Oct 18, 2015

While trying out the new pull request by @galsasson #4014 I found another bug in Tweak Mode when using more than one tab. Some decimals in my second tab were mysteriously ignored. After digging a bit around in SketchParser.java I found out that they are falsely accused of being inside one of the ignored functions (setup or settings). After looking a bit deeper into that ignore mechanism, which is based on ranges over characters I figured, that it is tab agnostic:

Assume our setup function spans char 5 to char 80 in the first tab, then numbers between char 5 to char 80 are ignored in every other tab! This was the reason why only some of my numbers were ignored and others not.

I am happy to work on a fix of this bug as soon as I get processing up and running in an IDE. I will let you know when I start the actual work so there is no double effort.

Edit: Working on it now ...

@galsasson

This comment has been minimized.

Show comment
Hide comment
@galsasson

galsasson Oct 18, 2015

Contributor

Hi @erniejunior, thanks for catching that bug and fixing it. Its impossible to see the changes you made to SketchParser.java, can you provide a pull request that doesn't replace the whole file?

Contributor

galsasson commented Oct 18, 2015

Hi @erniejunior, thanks for catching that bug and fixing it. Its impossible to see the changes you made to SketchParser.java, can you provide a pull request that doesn't replace the whole file?

@erniejunior

This comment has been minimized.

Show comment
Hide comment
@erniejunior

erniejunior Oct 18, 2015

Contributor

Hi @galsasson, I do not know why git does not show the individual changes. Maybe it has something to do with the fact, that I applied an automatic code formatter to comply with the Style Guide? That was probably a stupid idea ...

Basically I added a tab property to the Range class and the Range.contains method now also checks if the tabs are equal. Then I worked my way up to make any necessary changes for the rest of the code to work again. We certainly could use a more elaborate method to filter the actually used numbers and also remove some duplicate code (the changes I made right now require passing around the tab ID together with the tab content quite a lot which I think is ugly). But for now I wanted to respect your work and make only minimal changes to fix the bug as quickly as possible.

Is this enough information to understand the changes that I made or should I redo them without applying the code formatter?

Contributor

erniejunior commented Oct 18, 2015

Hi @galsasson, I do not know why git does not show the individual changes. Maybe it has something to do with the fact, that I applied an automatic code formatter to comply with the Style Guide? That was probably a stupid idea ...

Basically I added a tab property to the Range class and the Range.contains method now also checks if the tabs are equal. Then I worked my way up to make any necessary changes for the rest of the code to work again. We certainly could use a more elaborate method to filter the actually used numbers and also remove some duplicate code (the changes I made right now require passing around the tab ID together with the tab content quite a lot which I think is ugly). But for now I wanted to respect your work and make only minimal changes to fix the bug as quickly as possible.

Is this enough information to understand the changes that I made or should I redo them without applying the code formatter?

@galsasson

This comment has been minimized.

Show comment
Hide comment
@galsasson

galsasson Oct 18, 2015

Contributor

I would prefer to handle ignoreFunctions the same way I'm handling now commentBlocks instead of introducing 'tab' into the Range class. This would be more efficient and will require less changes to the code while keeping them local to SketchParset.java.
commentBlocks now keeps comments per tab. You will need to change ignoreFunctions to be the same data type, and initialize it correctly.
Then, in 'addAll***Numbers' you can replace:

        if (isInRangeList(start, ignoreFunctions)) {
          // ignore numbers in predefined functions
          continue;
        }

With:

        if (isInRangeList(start, ignoreFunctions.get(i))) {
          // ignore numbers in predefined functions
          continue;
        }

Where 'i' is the tab number

Thanks again!

Edit: and please provide a pull request without the code formatter, it would be many times easier maintaining the code in the future.

Contributor

galsasson commented Oct 18, 2015

I would prefer to handle ignoreFunctions the same way I'm handling now commentBlocks instead of introducing 'tab' into the Range class. This would be more efficient and will require less changes to the code while keeping them local to SketchParset.java.
commentBlocks now keeps comments per tab. You will need to change ignoreFunctions to be the same data type, and initialize it correctly.
Then, in 'addAll***Numbers' you can replace:

        if (isInRangeList(start, ignoreFunctions)) {
          // ignore numbers in predefined functions
          continue;
        }

With:

        if (isInRangeList(start, ignoreFunctions.get(i))) {
          // ignore numbers in predefined functions
          continue;
        }

Where 'i' is the tab number

Thanks again!

Edit: and please provide a pull request without the code formatter, it would be many times easier maintaining the code in the future.

erniejunior added a commit to erniejunior/processing that referenced this issue Oct 18, 2015

@erniejunior

This comment has been minimized.

Show comment
Hide comment
@erniejunior

erniejunior Oct 18, 2015

Contributor

Hi @galsasson thank you for your feedback. You are right, I should have studied your code more thoroughly before rushing with my changes. I just opened another pull request (#4023) that implements your suggestion. I hope this one is more transparent.

Contributor

erniejunior commented Oct 18, 2015

Hi @galsasson thank you for your feedback. You are right, I should have studied your code more thoroughly before rushing with my changes. I just opened another pull request (#4023) that implements your suggestion. I hope this one is more transparent.

benfry added a commit that referenced this issue Oct 20, 2015

Merge pull request #4023 from erniejunior/master
Improved fix for Issue #4017 where tweak mode ignores some numbers in a second tab

@benfry benfry added this to the 3.0.1 milestone Oct 20, 2015

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Oct 20, 2015

Member

All set for 3.0.1.

Member

benfry commented Oct 20, 2015

All set for 3.0.1.

@benfry benfry closed this Oct 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment