-
-
Notifications
You must be signed in to change notification settings - Fork 228
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
Conversation
- 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.
There was a problem hiding this 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).
|
||
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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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 :)
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>
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. |
Done addressing things, I'm going to see if I can figure out why CI is falling next. |
This reverts commit 8564d39.
Sorry for the spam commits, just wanted to test whether I can push to the PR :) |
no problem I've been learning what can be done too ;) |
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
|
Yeah I'll fix those :) |
I think we committed at the same time and some how mine because forced even though I didn't force it :\ |
No problem, you only did linter stuff right? I'll probably just force push over that, if that's ok :) |
Awesome can't wait to try/test it. I'm just heading out for a bit, I'll test it when I return though! |
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. |
Maybe something like http://angular-step-input.10kb.nl ? |
I have an idea for the numeric inputs that I"m going to try to work on this week. |
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? |
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. |
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. |
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 :) |
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! |
Sure take your time, doesn't really matter if this PR stays open for another day or two :) |
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. |
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! |
Damn, thought I had fixed it. Will try again later today, once that's fixed we're good to merge. |
I believe so!! |
Fixed, for me in both views now. |
🚢 |
* - 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>
Step 1 in #921