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

feat(ui): Add FORCE_MOVE support to tool controls #750

Merged
merged 11 commits into from
Jul 15, 2022

Conversation

kerbilg
Copy link
Member

@kerbilg kerbilg commented Jul 8, 2022

Closes: #494

Hi,
I am here with another PR 😁 I hope I have not misunderstood the request.

I have made a video again to demonstrate the function:

force_move-demo.mov

I have not included a warning, anyone who manually activates force_move should actually know the dangers. Personally, I would find it annoying. If it is still desired, I can work on it.

This PR is not complete and needs some css work. I've been working on it for days but failed, maybe someone else can help. There is a comment under Toolhead.vue.

You can see the css disaster by marking the background:
missing-css

If anyone has any questions about the code, please contact me on Discord: BastelKlug#5605

Signed-off-by: Kerim Bilgic [email protected]

@kerbilg
Copy link
Member Author

kerbilg commented Jul 8, 2022

Hi, I have fixed the style errors. In theory it is now adoptable.

So here is the current status:
update-force_move

@pedrolamas
Copy link
Member

Thanks for this @BastelKlug, I've just taken it for a quick test and it works very well! 😁

Having said that, I personally am not a fan of the override positioning...

I do wonder if it would look better as a button here:

image

If you look at the temperature charts, we have a button there that has "Chart Off/On", and I think the same approach can be taken here...

@matmen what do you think?

@matmen
Copy link
Member

matmen commented Jul 9, 2022

I agree, having it in the card header would be much better placement-wise. I also think that some kind of warning definitely is necessary here, optimally every time FORCE_MOVE is enabled, but I'm also fine with having a setting under General that shows/hides the checkbox in the tool card (which then displays a warning when it's toggled on). Maybe the latter is even a better option so we don't clutter "normal" users UIs?

@kerbilg
Copy link
Member Author

kerbilg commented Jul 9, 2022

Hi, I will work on the changes and upload them when they are ready.

@kerbilg
Copy link
Member Author

kerbilg commented Jul 12, 2022

Hi, I am back and have implemented the suggestions.

So here you can see the changes in action:
https://youtu.be/OSIAlxry9mk (sorry github only allows files less than 10mb)

@pedrolamas pedrolamas added this to the 1.19.1 milestone Jul 13, 2022
@pedrolamas pedrolamas added the FR - Enhancement New feature or request label Jul 13, 2022
Signed-off-by: Pedro Lamas <[email protected]>
Signed-off-by: Pedro Lamas <[email protected]>
@pedrolamas pedrolamas requested a review from matmen July 13, 2022 10:32
pedrolamas
pedrolamas previously approved these changes Jul 13, 2022
Signed-off-by: Pedro Lamas <[email protected]>
@pedrolamas
Copy link
Member

This is really great, thank you for your work here @BastelKlug! 🙂

I've now approved the PR, but made a couple of changes before we merge this down, specifically:

  • dropped the visibility toggle for FORCE_MOVE button (if it is enabled in printer.cfg, then always show)
  • some minor styling tweaks for consistency
  • added public to any Prop for proper lint alignment
  • refactored logic on toolhead buttons color and disabled states

@pedrolamas
Copy link
Member

pedrolamas commented Jul 13, 2022

I do wonder if we should change the movement buttons from the existing up/down/left/right arrows to minus/plus when FORCE_MOVE is enabled... any thoughts on that @BastelKlug @matmen ?

@kerbilg
Copy link
Member Author

kerbilg commented Jul 13, 2022

Bildschirmfoto 2022-07-13 um 12 49 10

Bildschirmfoto 2022-07-13 um 13 20 49

I think the old design with the arrows looks better 🤔

@pedrolamas
Copy link
Member

...but I'm also fine with having a setting under General that shows/hides the checkbox in the tool card (which then displays a warning when it's toggled on).

@matmen I notice now that you actually mentioned having a toggle button to show/hide the FORCE_MOVE button on the interface - the one I just removed (oops!)

I wonder if we actually need this toggle, given the button will only show if enabled in the printer.cfg (same as the QGL, bed screws, z_tilt, etc.)

@matmen
Copy link
Member

matmen commented Jul 13, 2022

My thoughts were that users that sometimes use FORCE_MOVE could disable the toggle for a less cluttered UI, but seeing the changes I don't really mind having it there anymore. I'll review this later :)

@matmen
Copy link
Member

matmen commented Jul 13, 2022

Ah! I remember why I wanted it 😉
image

@kerbilg
Copy link
Member Author

kerbilg commented Jul 13, 2022

Ah! I remember why I wanted it 😉 image

Oh I don't know if it's realistic, but if there are too many buttons, it should actually change to the mobile layout (with the dropdown menu) I had already noticed it when I was developing

@pedrolamas
Copy link
Member

pedrolamas commented Jul 13, 2022

I think in those cases, everything should just go to the overflow menu... guess we need to make a few changes here... but that is for a different matter to this topic! 🙂

Copy link
Member

@matmen matmen left a comment

Choose a reason for hiding this comment

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

Functionality wise it works fine, though I'm wondering it it maybe should be disabled when the printer is homed? Maybe I'm missing something here, but what's the use case of using FORCE_MOVE when the printer is already homed and can be moved - is it just for moving it out of range (that's what I use FORCE_MOVE for mostly)?

@pedrolamas
Copy link
Member

...maybe should be disabled when the printer is homed?

True, but if Klipper allows it, why should we limit the feature ourselves? It's not like we disable the homing buttons when the printer is homed either! 🙂

I know some users have overridden the default homing procedure and use FORCE_MOVE instead (I believe there's even an "advanced homing macro" somewhere, though I can't remember where I saw it now...)

@matmen
Copy link
Member

matmen commented Jul 15, 2022

...maybe should be disabled when the printer is homed?

True, but if Klipper allows it, why should we limit the feature ourselves? It's not like we disable the homing buttons when the printer is homed either! 🙂

True, fair enough 😂

@matmen matmen changed the title feat(ui): Add force_move support feat(ui): Add FORCE_MOVE support to tool controls Jul 15, 2022
@matmen matmen merged commit c6b9d8b into fluidd-core:develop Jul 15, 2022
@kerbilg kerbilg deleted the force_move branch August 8, 2022 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FR - Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

force_move toggle
3 participants