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

Added setting to change GCode-visualizer file size threshold to ui #1145 #1308

Merged
merged 1 commit into from May 9, 2016

Conversation

Projects
None yet
3 participants
@philphilphil
Copy link
Contributor

commented Apr 10, 2016

What does this PR do and why is it necessary?

It adds the ability to change the file size threshold of the gcode visualizer in the ui settings.
It also moved the gcode settings to a new tab.

How was it tested? How can it be tested by the reviewer?

I tested on my machine with multiple files and different settings. It can be tested with the virtual printer, adding some Gcode-Files with different file sizes and load them.

Any background context you want to provide?

What are the relevant tickets if any?

#1145

Screenshots (if appropriate)

image

Further notes

@gddeen

This comment has been minimized.

Copy link

commented Apr 10, 2016

I use Opera, FireFox, Safari, Chrome, and a linux browser.
I think Safari takes sizes much larger than others.
Maybe I need to test this, but I believe having a single desktop value wouldn't work in that case.

I assume the response might be "set it to 0 and you are no worse off?"

@philphilphil

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2016

The way it's implemented right now uses the exact number given, 0 would mean 0 bytes and no file would load automatically. If wanted i can implement that 0 means unlimited.
Setting up a value for each browser in use is, afaik, not possible because those values are stored on your octoprint server.

@foosel

This comment has been minimized.

Copy link
Owner

commented Apr 11, 2016

Could it be that you forgot to add a file to the commit? Only looking at the diff right now, but it seems like sizeObservable isn't declared anywhere, only used for the two size observables.

Also, I have to admit, I'm a bit unsure about the positioning of the value configuration. Looks a bit "misplaced" right within the feature checkbox army there, but I'm also a bit unsure what might be a better place... I wonder if it might make sense to extract the gcode visualizer settings (enable flag + those two settings) into their own tab under "Features" maybe?

@foosel

This comment has been minimized.

Copy link
Owner

commented Apr 11, 2016

m( never mind, the sizeObservable is already there in helpers.js, just forgot about it :)

@philphilphil

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2016

I think you are right. Making a new Tab for the GCode-Visiualizer makes more sense because it's a feature.. I'll add it.
What is the procedure here? Do i close this Pull-Request and open a new one with my changes?

@philphilphil

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2016

ah, never mind, i didn't knew github updates pull-requests when adding a new commit. I'll add my code tonight.

@philphilphil

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2016

I added the code to move the settings to a new tab works fine in safari, chrome and firefox

@foosel foosel merged commit 9316105 into foosel:devel May 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@foosel

This comment has been minimized.

Copy link
Owner

commented May 9, 2016

Now merged, sorry that it took me so long, that stupid funding situation is keeping my hands full these days.

@philphilphil philphilphil deleted the philphilphil:gcodeVisualizerFileSizeThreshold branch Sep 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.