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

Dev/gcode layer button #1306

Merged
merged 6 commits into from May 9, 2016

Conversation

@agarwali
Copy link
Contributor

commented Apr 8, 2016

Thank you for your interest into contributing to OctoPrint, it's
highly appreciated!

Please make sure you have read the "guidelines for contributing" as
linked just above this form, there's a section on Pull Requests in there
as well that contains important information.

As a summary, please make sure you have ticked all points on this
checklist:

  • Your changes are not possible to do through a plugin and relevant
    to a large audience (ideally all users of OctoPrint)
  • If your changes are large or otherwise disruptive: You have
    made sure your changes don't interfere with current development by
    talking it through with the maintainers, e.g. through a
    Brainstorming ticket
  • Your PR targets OctoPrint's devel branch (not master,
    maintenance or anything else)
  • Your PR was opened from a custom branch on your repository
    (no PRs from your version of master, maintenance or devel please),
    e.g. dev/my_new_feature
  • Your PR only contains relevant changes: no unrelated files,
    no dead code, ideally only one commit - rebase your PR if necessary!
  • Your changes follow the coding style
  • If your changes include style sheets: You have modified the
    .less source files, not the .css files (those are generated with
    lessc)
  • You have tested your changes (please state how!) - ideally you
    have added unit tests
  • You have run the existing unit tests against your changes and
    nothing broke
  • You have added yourself to the AUTHORS.md file :)

Feel free to delete all this help text, then describe
your PR further. You may use the template provided below to do that.
The more details the better!


What does this PR do and why is it necessary?

This pull request adds Layer Up/Down buttons to the Gcode viewer. It also fixes a bug by disabling key-bindings for layer up/down, which was requested in the same issue #1149

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

It was tested by connecting to 3-D printer and trying out all the possible cases like: 1) when g-code is not uploaded 2) when g-code is uploaded 3) when g-code is loaded and deleted

Any background context you want to provide?

The changes were automatically applied to the TouchUI plugin.

What are the relevant tickets if any?

#1149

Screenshots (if appropriate)

screenshot from 2016-04-04 00 18 38

Further notes

agarwali added some commits Apr 8, 2016

Added buttons to change layer in the gcodeviewer canvas, and fixed bu…
…g for layer up and down key-binding. It is now disabled when no file is uploaded
@@ -419,6 +419,9 @@ $(function() {
self.layerSlider.slider("disable");
self.layerSlider.slider("setMax", 1);
self.layerSlider.slider("setValue", 0);
$('#btn_layer_up').prop('disabled', true);

This comment has been minimized.

Copy link
@foosel

foosel Apr 11, 2016

Owner

Hm... While there are still some places within OctoPrint where there are such direct jquery references, in general it's less repetitive and cleaner to fetch those elements once and store them on the view model (e.g. like with self.layerSlider).

In that specific case here you manipulate the disabled property only (plus set a click handler further down below, see the comment to that), and I wonder if that wouldn't work better through a separate property on the viewmodel (e.g. self.layerSelectionEnabled = ko.observable(false)) and then setting both buttons' enabled binding to that within the HTML markup (e.g. data-bind="enabled: layerSelectionEnabled").

Would have the advantage to better separate view from view model (and in fact, the only reason the self.layerSlider element gets manipulated directly is because I didn't jump through the hoops of implementing a custom knockout binding for it, would also have been the cleaner approach actually and might be something to do in the future).

@@ -566,6 +553,38 @@ $(function() {
self.onTabChange = function(current, previous) {
self.tabActive = current == "#gcode";
};

self.incrementLayer = function(value){

This comment has been minimized.

Copy link
@foosel

foosel Apr 11, 2016

Owner

That's a bit of a misleading name - it doesn't actually increment but rather changes the layer. What about changeLayer instead?

}
};

$( "#btn_layer_up" ).click(function() {

This comment has been minimized.

Copy link
@foosel

foosel Apr 11, 2016

Owner

The click handler should rather be bound through knockout, so that this stays consistent with other parts of the application.

I suggest to define to methods incrementLayer and decrementLayer on self that do the job, then binding those to the buttons through data-bind="click: incrementLayer" and data-bind="click: decrementLayer".

<div id ="button_control" class="btn-group-toolbar" style="width: 100%; padding-bottom: 7px; padding-top: 5px;text-align: center;">
<button id = "btn_layer_up" type="button" class="btn btn-primary btn-medium" style="width:48%" disabled='disabled'>
<i class="icon-white icon-arrow-up"></i>
<span>Layer Up</span>

This comment has been minimized.

Copy link
@foosel

foosel Apr 11, 2016

Owner

The Layer Up text should be translatable. All you need to do for that to work is wrap the text in {{ _('...') }}, so {{ _('Layer up') }} and {{ _('Layer down') }} further below.

@foosel

This comment has been minimized.

Copy link
Owner

commented Apr 11, 2016

Great work! Sorry for just getting around to reviewing this...

I added a bunch of comments, could you take a look at them? It's only small stuff, don't worry.

eykrevooh and others added some commits Apr 12, 2016

Replaced jquery with ko functions e.g. the button clicks and enabling…
… button. Fixed an error in last commit: renamed a function that was already being used, self.changeLayer to self.shiftLayer.
@agarwali

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2016

@foosel we fixed all the things you mentioned above. We replaced jquery, with ko click and enable functions. Also, when we renamed self.incrementLayer with self.changeLayer, it was throwing an error because a function named self.changeLayer was already defined. So, we renamed it to self.shiftLayer instead. The jinja template is also changed to enable the ko. functions. The text on the layer button is now replaced with the specific jinja formating to allow it to be transalted.

@foosel

This comment has been minimized.

Copy link
Owner

commented Apr 18, 2016

@agarwali sounds good, I'll take a look as soon as possible. I currently have my hands somewhat full with handling the funding situation, but let me already say: very nice work!

@foosel foosel merged commit 5937c0c 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

Merged :)

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.