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

GUI: fix log (use document length instead of offset) #429

Merged
merged 1 commit into from May 21, 2018

Conversation

@arisona
Copy link
Contributor

arisona commented May 9, 2018

Background: Previously threw with JDK10, now works as expected.

Background: Previously threw with JDK10, now works as expected.
@arisona

This comment has been minimized.

Copy link
Contributor Author

arisona commented May 14, 2018

Any insights why CI fails? First build failed, but second went through...

@zrnsm

This comment has been minimized.

Copy link
Contributor

zrnsm commented May 14, 2018

Not clear what happened with the Linux build. It just timed out I think. I don't have write access to the main repo so I can't manually restart. The build will restart if you close and open the pull request though.

What is the exception that is thrown here in JDK10? Looking at the docs for 10, the original methods are still there. It seems like getLength() should be equivalent exactly like you have here though. I think the distinction is that if you hold a reference to the Position object that getEndPosition() returns, it will track the end position over time as edits are made and maybe this is useful if you have other Position objects around. But we were just discarding that reference immediately, getting the length should be the same thing.

I'm a little bit surprised that this is the only spot that fails under JDK10. As far as I know, there hasn't been a significant effort to target versions more recent than 8; that's sort of the current canonical API level for the project. But the build process ultimately uses whichever system java it finds; it doesn't enforce using a particular API level. I don't think that there are significant technical reasons why we can't target more recent versions. It's just going to take a bit of testing to find issues. We should be better about documenting exactly which versions are supported and how to build for them.

@arisona

This comment has been minimized.

Copy link
Contributor Author

arisona commented May 14, 2018

Thanks for the feedback - first of all, I don't wanna push anyone moving to a later JDK. I didn't notice any other issues so far, so I thought I will stick to JDK10. Downgrading would be a small thing for me.

Re issue at hand: getEndPosition().getOffset() initially returns 1, not 0. So trying to insert with this offset into the document threw an BadLocationException. Not sure why this behavior is different between the JDKs (tried googling a couple of days ago, but didn't find any concrete hints).

@arisona arisona closed this May 14, 2018
@arisona arisona reopened this May 14, 2018
@arisona

This comment has been minimized.

Copy link
Contributor Author

arisona commented May 14, 2018

Closed and reopened to restart build.

@zrnsm

This comment has been minimized.

Copy link
Contributor

zrnsm commented May 14, 2018

Wow, the 1, 0 behavior is pretty confusing. I was worried there would be some kind of off by one thing happening there. I'm going to do a little bit more digging to figure out why that might be happening.

@zrnsm

This comment has been minimized.

Copy link
Contributor

zrnsm commented May 14, 2018

What I've found so far: getEndPosition().getOffset() seems to always be length + 1. In Java 8 inserting beyond the end just inserts at the end. So when we try to to insert at index 1 but the doc length is 0 it doesn't throw an exception. Java 9 and higher will throw.

A zero length document has a start position of 0 and an end position of 1. And then inserting at the end position doesn't work. Confusing. Relevant part of the documentation: https://docs.oracle.com/javase/7/docs/api/javax/swing/text/Position.html#getOffset()

Inserting at getLength is what we want here going forward. Also, this is the only place where we call getEndPosition, so this commit should be sufficient.

@JohannesTaelman JohannesTaelman merged commit 8fcc99f into axoloti:experimental May 21, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JohannesTaelman

This comment has been minimized.

Copy link
Collaborator

JohannesTaelman commented May 21, 2018

Thanks!
I'm hesitant to upgrade to JDK 9 or 10 as they will have a very short lifetimes, and seem to drop support for older systems (32bit...). Yet it is good to prepare for the future...

@zrnsm zrnsm mentioned this pull request Jun 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.