Experimental branch merging #108 #112 #115

[gb0101010101]

Merged #108 #112 #115 into experimental branch as requested in https://github.com/madgrizzle/WebControl/pull/115#issuecomment-602245622

Please be sure to close these pull requests when this has been implemented. I have omitted #116 as I think this may need further discussion and additional changes. I will comment on thoughts there.

Merge conflicts on #108 which were easily resolved. Tested all z-axis functionality and it worked which is the only thing that could have broken. Tested all other UI button controls which worked as expected.

[madgrizzle]

Re: logger.. iirc, the reason I had to do it the way I did was that the file might be being written to at the same time that the program is trying to delete the file. I think this occurred during testing it so I put it in a try loop and pounded it 1000 times. Probably the correct implementation would be to set a flag to disable logging, wait until its disabled, clear the logs, then re-enable logging.

[madgrizzle]

Didn't forget about this. Got busy.

[madgrizzle]

What I'm thinking is perhaps we start doing 'stable' builds with the last digit even and 'experimental' builds with the last digit odd. We could add a setting to Maslow Settings to allow the user to see experimental builds, otherwise, they get filtered from the list that ReleaseManager produces. We'll still label it as Experimental in the title, but I don't think we'd have to do too much more than that.

[gb0101010101]

Normally experimental code is kept on a "Development" branch (rather than Master). Could add a setting to select a branch (Development, Stable). The update check could just check the different branch. You could also add "-dev" to the tags that are experimental.

[gb0101010101]

Hey, I though there was a release for the Experimental branch? Can find it any more.

[madgrizzle]

There was, but the release manager was broken in it so once you upgrades to it, you couldn't get out.. so I deleted it as well as all versions since late January that had the same issue.

[madgrizzle]

Well, I made a royal mess. I tried to make a PR that merged changes from master branch into the experimental branch. I just wanted to see what it would do and it looked good (it seemed to bring only the recent changes in, to my surprise). But little did I know.. or notice.. that in the process of doing so, it merged the experimental branch into the master branch and THEN the generated the PR to bring the recent changes to the master branch into the experimental branch. So both my master branch and experimental branches were the same. I had wanted to leave the master branch as-is. I couldn't find a way to revert the commit to the master branch (wasn't a PR that could be reverted and that's all I could figure out how to do). So, I ended up downloading the source to release 0.932, copying it over my local repo files, and committing it all back.

So, what I need to do is to just apply the same commits to the master branch to the experimental branch.. and I assume that's not easy and assume that's why you commented about not making more changes to the master branch :)

Sigh.

[gb0101010101]

It sounds like you did this on GitHub rather then using a local Git copy. I basically don't trust the site to do anything but a simple PR merge and do everything using command line on local copy. Then only push changes to Github when absolutely positive everything is correct. There are various Git GUIs that make things easier at the beginning but you can't beat CLI for full control. Using a local copy provides a safety buffer and gives greater functionality.

As long as you are working on local git clone and have not pushed commits to Github then you can easily revert local commits DELETING ALL CHANGES using:
git reset --hard HEAD@{N}
Where N is the number of local commits you want to got back. Use git log to figure out how many.

DO NOT reset a repo in a collaborative environment (GitHub) as you are rewriting history and it will break other peoples forks and PRs. That is why it is always good to work on a local clone.

Here is a really good guide on undoing changes
https://sethrobertson.github.io/GitFixUm/fixup.html

Let me know if you want some help on setting up local copy.

[madgrizzle]

It wasn't a local commit, it happened on the github server via the webpage. Normally I don't have an issue with PRs, just this one did something unexpected. I told it to create a PR to bring changes from master into experimental, but because experimental had commits ahead of the master, I didn't notice it merged those commits into the master and then created the PR.

Is there a method to pull just a commit of one branch (i.e., master) into another branch (i.e., experimental)?

[gb0101010101]

Is there a method to pull just a commit of one branch (i.e., master) into another branch (i.e., experimental)?

AFAIK you can't do that directly on GitHub. Using CLI on local repo you can git cherry-pick <commit hash> to merge one commit into current working branch. FYI you don't need the full hash; normally 5 or so characters will be enough. Usegit log to see the commit hash.
https://git-scm.com/book/en/v2/Distributed-Git-Maintaining-a-Project#_rebase_cherry_pick

[gb0101010101]

@WebControlCNC/developers @WebControlCNC/maintainers

I have merged this PR with latest master and tested everything I could think of. It is now ready for y'all to test. I really want this tested and committed before other new features as it greatly changes HTML, CSS, JavaScript, and Python actions.py file. Any new features that change UI or add Actions, and work off current code, will have merge conflicts with this PR making it harder to commit later on.

Issues not related to this PR

1. Linux KDE KIO Client no longer opens browser window when running/debugging application. It looks like its passing /localhost:5000 which is not recognized as URL. Perhaps related to the HTTP/HTTPS issue mentioned recently (can't remember where). This may affect other OSes.
2. releaseManager.py code fails parsing release.tag_name to float on latest test release because of the new date formatted text.

Things to test:

When connected to WebControl in browser please make sure to press keys [Ctrl] + [F5] to force reload CSS and JavaScript files (or clear browser cache). Otherwise layout will be messed up and button action may not function.

1. The layout should now fit desktop browser without any window scroll bars. The 'Controller Messages' area should have a Y axis scroll bar. Mobile will always have Y-Scroll.
2. Menus should fit/work on desktop and mobile.
3. Render Canvas (left area) should resize fluidly on desktop, without lag, maintaining current zoom level.
4. Z-Axis button modal has new buttons "goto safe" and "goto zero" which work using settings.
5. All UI buttons should function. This was a big rewrite that greatly simplified code.

If you see a message like "Function not implemented: [function_name]" then let me know.

New addition

5c9377c removes class text-nowrap on #gcodeLine container because very long gcode causes the right column to change in width. I think it is much better for the lines to wrap, shifting subsequent items up/down, rather than having it change the column width, which resizes the whole layout and makes buttons difficult to click. Controller Messages area already warps lines like this and should resize in height, keeping lines visible, if this happens.

Thanks @emilecantin for closing #153.

Please let me know if I am doing anything wrong, especially with GitHub, as I am new to to it, and may be using antiquated GIT workflows that are not acceptable.

[gb0101010101]

HTTP/HTTPS issue mentioned in #146 [committed] that might be causing KDE KIO client issue.

[zaneclaes]

Ah, I think it's the line webbrowser.open_new_tab("//localhost:"+webPortStr) in main.py. I thought that served a different purpose. It could be restored to http://localhost:". The other two changes, which change the socket.io` connection, seem to be sufficient.

[gb0101010101]

#157 fixes webbrowser.open_new_tab(). Thanks @zaneclaes for pointing it out.

[gb0101010101]

I would really like to see these extensive changes committed as I have many other changes that depend on them.

[rolambert]

Code looks good, changes looks good let me test this out some more.

[rolambert]

I found no issue

[gb0101010101]

Thx for the review @rolambert

[nhogle]

@gb0101010101: I noticed a bug with this branch which prevents the acceptTriangularCalibrationResults. Please see this commit on my own branch:
ae97180

I haven't figured how to submit a pull request to this existing pull request...

EDIT: It looks like holeyCalibration would have the same issue, please see this line in holeyCalibration.html:
https://github.com/gb0101010101/WebControl/blob/5c9377cd7a9bb706cd29508d1a56e084120d7f94/templates/holeyCalibration.html#L103

[gb0101010101]

@nhogle Thanks for the input. Both acceptTriangularCalibrationResults(self) and acceptHoleyCalibrationResults(self) do not accept additional arguments, beyond 'self', so calling action('acceptHoleyCalibrationResults',0,0) is wrong, as the additional arguments 0,0, should not be passed, and do nothing.

[nhogle]

@nhogle Thanks for the input. Both acceptTriangularCalibrationResults(self) and acceptHoleyCalibrationResults(self) do not accept additional arguments, beyond 'self', so calling action('acceptHoleyCalibrationResults',0,0) is wrong, as the additional arguments 0,0, should not be passed, and do nothing.

@gb0101010101 : Right, that is why I was proposing that the Javascript that invokes these methods (see here and here) should not be passing extra parameters. The processAction method in the Actions class passes whatever parameters that it receives in the incoming message to the command (method) it retrieves via getattr. If Python tries to pass the wrong number of parameters to a function, we get an exception.

I do think this revised processAction method is a lot cleaner than what we see in the master branch,, (a big if/elif ladder) so I would like to see this code get merged somehow. By now, is it too late? Has master drifted too far?

[Orob-Maslow]

I like the simplification, but it is not ready to merge. There are a couple embedded if statements get skipped over now. For example, the definehome if statement pulls a couple arguments for the queue then sets the home position. Startrun has an extra if statement as does the cleargcode if statement. This new simplification just puts the command on the queue without these procedural checks or execution. Somewhere these extra statements need to be put in code without losing the message data. In the case of the new coordinates of define home, that would need to be passed along as an argument, but not every statement needs an argument as mentioned above.

[Orob-Maslow]

For the conflict with frontpage files, the button block on the screen is useful for the user and could save some realestate on the screen during runtime, but the controller, while in run mode, will ignore commands that are not allowed during run time. I've hit these at times with my fat fingers on my small phone and a message dialog box pops up and says the command is not allowed while the machine is running. Do we need both?