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

Bounds checking and zoom #137

Merged
merged 5 commits into from
Aug 23, 2016

Conversation

KalKolberg
Copy link
Contributor

Fixing out of bounds error when displaying pdf preview and reverting scaling to previous version.

In reference to #131 and #130 unfortunately does not solve the out of memory error with #33. The out of memory issue arises with the old zoom code as well. Will have to look into it further.

@KalKolberg
Copy link
Contributor Author

F823030 deals with the OutOfMemoryError that was being generated before causing a graceful failure when program exceeds the heap size and replaces pdf preview with a blank screen. This effectively fixes #33 Increasing the heap size of java will give more room to zoom but when dealing with an uncompressed image we are limited in what we can do as the image grows exponentially as the size increases.

ImageIcon icon = new ImageIcon(generatedImage);
JLabel picLabel = new JLabel(icon);
pdfImagePanel.add(picLabel);
} catch (OutOfMemoryError oome) {
Copy link
Owner

Choose a reason for hiding this comment

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

Catching OutOfMemoryError is pointless. All it really does is preventing the stacktrace from being shown to the user but we cannot predict the system's behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't image just throwing an OutOfMemoryError and returning Null? I'm working on figuring out how to increase the heap size in the executable currently, maven is new to me, the limit of 64m is fairly low with the size of uncompressed images and pdfs so increasing that should help out.

Copy link
Owner

Choose a reason for hiding this comment

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

If it throws an error, it immadiately aborts any further execution of the code.
It seems that maven compiler plugin allows modifying the memory:
https://maven.apache.org/plugins/maven-compiler-plugin/examples/compile-with-memory-enhancements.html
If not that, perhpaps setting the proper value via MAVEN_OPTS on the dev machine is the way to go. Do you want to check on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check it out. As far as the error goes, check me on this, but doesn't the Image class throw an error because of the image file and we catch the error here and disregard it. The image never gets made and the image returns a null which displays as a blank screen on the preview. We can't make it work with the current memory limit but this way it doesn't present an error the preview just goes away. Btw with the image then cleared the program usually has enough memory to display the next bump. With my system it's currently crashing at 190 but then can display 200 without a problem. Other than increasing the memory how else would we deal with this? This is the first serious java project I've been on so just trying to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The https://maven.apache.org/plugins/maven-compiler-plugin/examples/compile-with-memory-enhancements.html code doesn't work directly, I imagine there is some code elsewhere that needs to be included to activate it. Will study it more tomorrow.

Copy link
Owner

Choose a reason for hiding this comment

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

So, can we remove this catch now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll go in an remove it

On Thu, Aug 18, 2016 at 3:45 AM, Sebastian Brudziński <
notifications@github.com> wrote:

In OpenLaTeXStudio/Editor/src/main/java/latexstudio/editor/
pdf/PDFDisplay.java
#137 (comment)
:

@@ -36,9 +36,13 @@ public JPanel drawPreviewOnJPanel() {
Image generatedImage = PDFPreviewBuilder.buildPDFPreview(selectedPage, viewZoom);

     if (generatedImage != null) {
  •        ImageIcon icon = new ImageIcon(generatedImage);
    
  •        JLabel picLabel = new JLabel(icon);
    
  •        pdfImagePanel.add(picLabel);
    
  •        try {
    
  •            ImageIcon icon = new ImageIcon(generatedImage);
    
  •            JLabel picLabel = new JLabel(icon);
    
  •            pdfImagePanel.add(picLabel);
    
  •        } catch (OutOfMemoryError oome) {
    

So, can we remove this catch now?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/sebbrudzinski/Open-LaTeX-Studio/pull/137/files/f823030ef76becab3cb924455ad2916bad99a496#r75261635,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAuhv4lWqz6TsGGYjJ6w1IhU0ogkzaUMks5qhA2YgaJpZM4JermW
.

@KalKolberg
Copy link
Contributor Author

KalKolberg commented Aug 9, 2016

Increased the size of the Heap to 1024m should help out with the out of memory errors we've been having due to the awt Image manipulation. This should effectively close ticket #33

Details of fix are covered here:
From the POM file, it looks like a NetBeans application which uses the
nbm-maven-plugin? So maybe this blog post helps you?

https://netbeansscribbles.wordpress.com/2013/11/27/changing-vm-options-for-netbeans-rcp-applications/

@KalKolberg
Copy link
Contributor Author

Okay, think I've covered about all I can with this branch. Would appreciate getting it merged into master asap. Going to start a new branch to work on other issues.

@sebbrudzinski
Copy link
Owner

I didn't test it - only reviewed the code, but since this is a pull request against the development branch, I'm happy to merge this. We will need to submit the whole dev branch as a pull request though.

@sebbrudzinski sebbrudzinski merged commit 425a901 into sebbrudzinski:dev/new-pdfbox Aug 23, 2016
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.

None yet

2 participants