Fix for Bug 229 + Only install interpreter if it is selected #93

Merged
merged 5 commits into from Oct 30, 2013

Conversation

Projects
None yet
3 participants
Contributor

jonahkichwacoders commented Oct 28, 2013

This is a proposal to resolve the issues being discussed on the mailing list.

This is not working well for me (error below):

The problem is that the size of the operations may be different from the size of the providers (when there are already some interpreters configured -- so, if there's one interpreter configured and the last of the list is chosen, the error will be given -- and depending on the situation, it may get the wrong operation for a given provider).

java.lang.IndexOutOfBoundsException: Index: 3, Size: 2
at java.util.ArrayList.rangeCheck(Unknown Source)
at java.util.ArrayList.get(Unknown Source)
at org.python.pydev.ui.pythonpathconf.AutoConfigMaker.autoConfigSearch(AutoConfigMaker.java:360)
at org.python.pydev.ui.pythonpathconf.AbstractInterpreterEditor.getNewInputObject(AbstractInterpreterEditor.java:907)
at org.python.copiedfromeclipsesrc.PythonListEditor.addPressed(PythonListEditor.java:132)
at org.python.copiedfromeclipsesrc.PythonListEditor$1.widgetSelected(PythonListEditor.java:227)
at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:248)

@jonahkichwacoders Actually, I've been working on fixes to this class to hopefully avoid index errors and other issues. Once it's ready I'll put a pull request on your repo, as you know what to look for more than I do, and I'd appreciate an extra pair of eyes.

Contributor

jonahkichwacoders commented Oct 29, 2013

I think I have addressed all the exit points from that loop now to ensure that they matchup.

Andrew, I look forward to your updates and will gladly review them.

Contributor

jonahkichwacoders commented Oct 29, 2013

Fabio, if you accept this change, can you publish another nightly build. I want to roll it out to Diamond ASAP?

Thanks,
Jonah

Contributor

jonahkichwacoders commented Oct 29, 2013

Fabio, please don't apply this pull request yet, see jonahkichwacoders#1

Owner

fabioz commented Oct 29, 2013

Ok, I'll wait for your sign to take a look and merge things :)

@jonahkichwacoders jonahkichwacoders Rewrite autoConfigSearch to be more succinct
Also remove the use of multiple arrays that need to be the same length,
instead replace it with a class that encapsulates all the info.
b824362
Contributor

jonahkichwacoders commented Oct 30, 2013

@AndrewFerr and @fabioz can you please review/test this change. I would like to go with this version for the auto-config changes. I have incorporated all the design changes that Andrew did, I have simply rewriten autoConfigSearch to be more succinct and also removed the use of multiple arrays that need to be the same length,instead replace it with a class that encapsulates all the info

Owner

fabioz commented Oct 30, 2013

Just reviewed and did some basic testing (on windows) and it seems good to me.

@AndrewFerr is it ok to apply as is?

Contributor

AndrewFerr commented Oct 30, 2013

@fabioz It's looking good! This is a big improvement. I still need some time to do some testing/fixes, though (but I don't think I'll be changing anything major).

Contributor

jonahkichwacoders commented Oct 30, 2013

Hi @AndrewFerr, great news. Are there any fixes to this version you know that need to be done? I am particularly interested in serious/critical bug fixes.

@fabioz, if you can make an RC build once this is merged I can roll that out to Diamond. Thanks!

Contributor

AndrewFerr commented Oct 30, 2013

@jonahkichwacoders I don't see any serious problems at the moment, maybe just some UI things that need some touching up.

Also, I have a quick question: is there a reason PossibleInterpreter doesn't have a property for its "name and executable" value? Right now it's calling getNameAndExecutable multiple times without storing the result.

Contributor

jonahkichwacoders commented Oct 30, 2013

@AndrewFerr the name and exec may be changed by being installed. It may be possible to recalculate it only if needed, but that seems to complicate the code for no measurable improvement.

@fabioz fabioz added a commit that referenced this pull request Oct 30, 2013

@fabioz fabioz Merge pull request #93 from jonahkichwacoders/auto_config_changes
Fix for Bug 229 + Only install interpreter if it is selected
2e5e85a

@fabioz fabioz merged commit 2e5e85a into fabioz:development Oct 30, 2013

1 check failed

default The Travis CI build could not complete due to an error
Details
Owner

fabioz commented Oct 30, 2013

Ok, just merged. I'll take a look at #96 and do the build once that's merged.

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