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

Reduce input during on-boarding #954

Merged
merged 51 commits into from
Sep 16, 2020
Merged

Conversation

TheSin-
Copy link
Contributor

@TheSin- TheSin- commented Aug 29, 2020

- Add mdns and ngx-electron plugin
- Add electron-rebuild to dev depends so that electron can be rebuilt with the installed node
- Fix setTimeout and setInstance to with with Node modules (required for mdns)
- Reduce the version on node as electron needs to be on the same version for this to work
- Change order in on boarding wizard to select API first
- Use mdns to detect and list octoprint instances (WIP need to figure out min version required)
- Auto set Printer name if a detected install if found.

Step 1 in #921

- Add electron-rebuild to dev depends so that electron can be rebuilt with the installed node
- Fix setTimeout and setInstance to with with Node modules (required for mdns)
- Reduce the version on node as electron needs to be on the same version for this to work
- Change order in on boarding wizard to select API first
- Use mdns to detect and list octoprint instances (WIP need to figure out min version required)
- Auto set Printer name if a detected install if found.

I'm trying to reduce input and the need for a keyboard during onboarding.  Obviously this is just the first step as the API key still needs to be inputed.  But it's still a start.

Follow Issue UnchartedBull#921 for more info on the changes.
Copy link
Owner

@UnchartedBull UnchartedBull left a comment

Choose a reason for hiding this comment

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

Looks really good. The only bigger point to discuss here, is whether the mdns part should be moved to the main.js, where we already have full access to the node api, sharing data can be easily done via the electron IPC. And the API Connection check can be at the end I guess. Everything else is just some minor codestyle stuff (I guess the pipeline will also complain about some of that).

src/app/config/no-config/no-config.component.ts Outdated Show resolved Hide resolved
src/app/config/no-config/no-config.component.ts Outdated Show resolved Hide resolved
src/app/config/no-config/no-config.component.ts Outdated Show resolved Hide resolved
Comment on lines 155 to 173

const mdns = this._electronService.remote.require('mdns');
const browser = mdns.createBrowser(mdns.tcp('octoprint'));
browser.on('serviceUp', service => {
var node = {
'display': service.name.match(/"([^"]+)"/)[1] + ' (' + service.txtRecord.version + ')',
'name': service.name.match(/"([^"]+)"/)[1],
'version': service.txtRecord.version,
'url': service.host.replace(/\.$/, '') + ":" + service.port + service.txtRecord.path.replace(/\/$/, '') + "/api/",
// Compare version to make sure it meets the requirement
'disable': false
};

this.octoprintNodes[service.host.replace(/\.$/, '').replace('.', '_')] = node;
});
browser.on('serviceDown', service => {
delete this.octoprintNodes[service.host.replace(/\.$/, '').replace('.', '_')];
});
browser.start();
Copy link
Owner

Choose a reason for hiding this comment

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

Can we put this in main.js, this would remove the ngx-electron dependency and the icp links are already there, plus we would get around having to add node to the main angular app.

Copy link
Owner

Choose a reason for hiding this comment

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

Let me know if you want me to do this.

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 that is beyond me, this took me long enough to figure out ;)

Copy link
Owner

Choose a reason for hiding this comment

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

Ok I'll take that one :)

src/app/config/no-config/no-config.component.ts Outdated Show resolved Hide resolved
src/app/config/no-config/no-config.component.ts Outdated Show resolved Hide resolved
src/app/config/no-config/no-config.component.ts Outdated Show resolved Hide resolved
src/app/config/no-config/no-config.component.html Outdated Show resolved Hide resolved
src/app/config/no-config/no-config.component.ts Outdated Show resolved Hide resolved
src/app/config/no-config/no-config.component.ts Outdated Show resolved Hide resolved
TheSin- and others added 4 commits August 31, 2020 07:16
Returns void

Co-authored-by: Timon G. <timon.gaebelein@icloud.com>
Return Boolean

Co-authored-by: Timon G. <timon.gaebelein@icloud.com>
Returns void

Co-authored-by: Timon G. <timon.gaebelein@icloud.com>
@TheSin-
Copy link
Contributor Author

TheSin- commented Aug 31, 2020

Because of my changes to post install I might need to move the electron-rebuild dev depend into depends? I'm really new to electron.

@TheSin-
Copy link
Contributor Author

TheSin- commented Aug 31, 2020

Done addressing things, I'm going to see if I can figure out why CI is falling next.

@UnchartedBull
Copy link
Owner

Sorry for the spam commits, just wanted to test whether I can push to the PR :)

@TheSin-
Copy link
Contributor Author

TheSin- commented Aug 31, 2020

no problem I've been learning what can be done too ;)

@TheSin-
Copy link
Contributor Author

TheSin- commented Aug 31, 2020

The new Travis fails is due to the changes in .eslintrc.js

Which is out side of my knowledge

https://travis-ci.com/github/UnchartedBull/OctoDash/jobs/379614533

Error: .eslintrc.js#overrides[0]:
608	Environment key "typescript" is unknown

@UnchartedBull
Copy link
Owner

Yeah I'll fix those :)

@TheSin-
Copy link
Contributor Author

TheSin- commented Aug 31, 2020

I think we committed at the same time and some how mine because forced even though I didn't force it :\

@UnchartedBull
Copy link
Owner

No problem, you only did linter stuff right? I'll probably just force push over that, if that's ok :)

@TheSin-
Copy link
Contributor Author

TheSin- commented Sep 4, 2020

Awesome can't wait to try/test it. I'm just heading out for a bit, I'll test it when I return though!

@TheSin-
Copy link
Contributor Author

TheSin- commented Sep 5, 2020

I'm searching like made but in electron is there no way to show the number input spinner ? That would be the best to keep it 100% touch only.

@TheSin-
Copy link
Contributor Author

TheSin- commented Sep 5, 2020

Maybe something like http://angular-step-input.10kb.nl ?

@TheSin-
Copy link
Contributor Author

TheSin- commented Sep 8, 2020

I have an idea for the numeric inputs that I"m going to try to work on this week.

@TheSin-
Copy link
Contributor Author

TheSin- commented Sep 11, 2020

This is my idea, but the two way data bind is really really slow to update and I can't figure out why. It needs to be much faster to be usable, is this due to electron?

@TheSin-
Copy link
Contributor Author

TheSin- commented Sep 11, 2020

Screen Shot 2020-09-11 at 1 48 47 PM

It's something with the IPC calls, if I set page to 4 and start it (skip the api calls) then it's super responsive and very nice to use. If I start at page 0 then it's pretty much unusable. Since I really do not understand the IPC stuff I think you might need to debug it.

@UnchartedBull
Copy link
Owner

Hi, sorry last week has been kinda busy. I'll have a look at that today or tomorrow :) Is there an option to enter text manually though? I think it will be really hard to get to the right number on 3.5" device. I guess we can change that now as well but all other text fields should be updates as well to the new input method. That will be tracked in #762 though.

@TheSin-
Copy link
Contributor Author

TheSin- commented Sep 14, 2020

I figured we could make the number display (span) into an input, or we could have a toggle to go between the 2. But I didn't want to work on it if I couldn't get it working at all.

@UnchartedBull
Copy link
Owner

Just tried it and it works just fine for me. No noticable lag at all (I started on page 0). It looks nice and certainly is a super clean way of inputting a number. The main problem here is, that some (possibly many) people are using 3.5" GPIO displays with something close to 1 or 2Hz refresh rate. This makes nice gestures for essential stuff almost impossible.

I redid those so we have + und - buttons (hold them to go up/down faster). I'm fairly happy with that, but please provide feedback on that. Sorry for crashing everything again. This PR is ready to be merged into master from my side now (if the pipeline passes). Let me know what you're thinking :)

@TheSin-
Copy link
Contributor Author

TheSin- commented Sep 15, 2020

I'll try them in a few minutes, just have a few meetings first. But if it works well I think that is the last of it, I'm excited!! This is a great update indeed!

@UnchartedBull
Copy link
Owner

Sure take your time, doesn't really matter if this PR stays open for another day or two :)

@TheSin-
Copy link
Contributor Author

TheSin- commented Sep 15, 2020

  • Looks good, is the alignment issue on purpose or is it just cause I'm running it on my Mac?

  • Are we going to add it to the Interval on Page 5 as well? Just to keep things consistent?

Screen Shot 2020-09-15 at 8 51 28 AM

@UnchartedBull
Copy link
Owner

That layout issue shouldn't happen. Some weird CSS thing going on with the input I guess. Should be fixed now though. The value interval won't receive the buttons since it will be removed soon (once the push updates are in place there is really no need for that anymore), so we can safe ourself the hassle of adding the controller there to.

@TheSin-
Copy link
Contributor Author

TheSin- commented Sep 15, 2020

yeah I was on the fence about cause really it shouldn't normally be changed. I'll try the new changes but otherwise I'm gtg on this, it's amazing work!

@TheSin-
Copy link
Contributor Author

TheSin- commented Sep 15, 2020

Odd it fixed the large version but not the smaller version

Screen Shot 2020-09-15 at 9 10 55 AM

Screen Shot 2020-09-15 at 9 11 04 AM

@UnchartedBull
Copy link
Owner

UnchartedBull commented Sep 15, 2020

Damn, thought I had fixed it. Will try again later today, once that's fixed we're good to merge.

@TheSin-
Copy link
Contributor Author

TheSin- commented Sep 15, 2020

I believe so!!

@TheSin-
Copy link
Contributor Author

TheSin- commented Sep 15, 2020

Fixed, for me in both views now.

@UnchartedBull
Copy link
Owner

🚢

@UnchartedBull UnchartedBull merged commit 6458cc6 into UnchartedBull:master Sep 16, 2020
kantlivelong pushed a commit to kantlivelong/OctoDash that referenced this pull request May 5, 2021
* - Add mdns and ngx-electron plugin
- Add electron-rebuild to dev depends so that electron can be rebuilt with the installed node
- Fix setTimeout and setInstance to with with Node modules (required for mdns)
- Reduce the version on node as electron needs to be on the same version for this to work
- Change order in on boarding wizard to select API first
- Use mdns to detect and list octoprint instances (WIP need to figure out min version required)
- Auto set Printer name if a detected install if found.

I'm trying to reduce input and the need for a keyboard during onboarding.  Obviously this is just the first step as the API key still needs to be inputed.  But it's still a start.

Follow Issue UnchartedBull#921 for more info on the changes.

* Add postinstall so it auto rebuilds electron against node

* Update src/app/config/no-config/no-config.component.ts

Returns void

Co-authored-by: Timon G. <timon.gaebelein@icloud.com>

* Update src/app/config/no-config/no-config.component.ts

Return Boolean

Co-authored-by: Timon G. <timon.gaebelein@icloud.com>

* Update src/app/config/no-config/no-config.component.ts

Returns void

Co-authored-by: Timon G. <timon.gaebelein@icloud.com>

* Resolve a few issues with VSCode and more consistent var naming

* Add version compare to disable older Nodes

* Switch back to 5000 as the default and add a note about port 80

* test

* Revert "test"

This reverts commit 8564d39.

* Attempt to fix CI

* restructure main.js

* With the new changes newer node is working again

* Use proper indentation

* Forgot to update package-lock with node update

* electron stuff finished

* Fix lint errors and warnings

* finalise workflow

* small bugfix

* prevent the same instance showing multiple times

* remove node from angular app

* Manually trigger change detection

* Also trigger change detection for Page changes to fix bug

* intial setup to load op script

* Fix ngZone issue

* Remove ngx-electron from package-lock

* Remove electron-rebuild, postinstall script and test if the travis changes are still needed since we are no longer rebuilding

* Tested and no longer required, was a rebuild only issue

* fix OctoPrint var

* finally get that freakin jQuery working

not pretty, but it works

* cleanup + reset URL if manual input is opened.

* Check OctoPrint connection and load client library

* Update package-lock, fix a few navigation bugs

* Make sure pages can't go below 0 or above totalPages

* refactor navigation controller + show next button after page 1 again

* login with request working

* use printer name from printerprofile

* fix setup (all working)

* Convert to range slider to numeric input is not required

* I think 150 would be fast enough to load

* value selector for numbers

* fix layout issue

* Fix alignemnt on large and small views

Co-authored-by: Timon G. <timon.gaebelein@icloud.com>
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.

2 participants