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

Expand Blockly with new blocks (part 1) & update to latest version #1179

Merged
merged 23 commits into from
Nov 7, 2021

Conversation

stefan-hoehn
Copy link
Contributor

@stefan-hoehn stefan-hoehn commented Oct 16, 2021

This is the first step into adding many more openhab blockly blocks.

  • add a structured menu with sub groups: audio, voice, timers, variables, notifications
  • add code documentation, tooltips and help urls
  • refactoring

Details can be found in the following thread:
https://community.openhab.org/t/extending-blockly-with-new-openhab-commands/127169

stefan-hoehn and others added 4 commits April 7, 2021 11:46
Signed-off-by: Stefan Höhn <stefan@andreaundstefanhoehn.de>
Signed-off-by: Stefan Höhn <stefan@andreaundstefanhoehn.de>
Signed-off-by: Stefan Höhn <mail@stefanhoehn.com>
Signed-off-by: Stefan Höhn <stefan@andreaundstefanhoehn.de>
@stefan-hoehn stefan-hoehn requested a review from a team as a code owner October 16, 2021 22:12
@hmerk
Copy link

hmerk commented Oct 17, 2021

@stefan-hoehn Could you please resolve the conflicts and remove the old commits not related to this blockly PR.

@stefan-hoehn
Copy link
Contributor Author

stefan-hoehn commented Oct 18, 2021

HI Hans-Jörg,

Can you guide me doing that?

  • it has caught me by surprise that the PR contains commits that are already pushed and merged. How can I get rid of these?
  • How do I solve that conflict?

Usually if I run into those kind of git issues, my feeling is to "start from scratch..." ?

Would the easiest way to do the following?

  • Rollback my last commit with the changes to become uncomitted? (is this possible after a push?)
  • Create a patch file of the changes.
  • Close this PR
  • Clone a complete new version of the main branch
  • Apply the patch (and do the merge)
  • Create a new PR

Even worse there is something weird with my repo compared to openhab:

  • I am 3 commits ahead and many commits behind.
  • I tried to "fetch upstream" and then pull but it doesn't solve the problem.
  • I wonder if the best would be to just save my changes, delete the repo and clone a new one?

@hmerk
Copy link

hmerk commented Oct 18, 2021

Hi Stefan,
sorry, but I am no git expert, so can't really tell.
I think you need to bring your main branch "up2date" and then rebase your enhancement branch...

@asmigala
Copy link

asmigala commented Oct 18, 2021

@stefan-hoehn the easiest way would be

git checkout blockly_enhancements_1
git fetch origin
git rebase -i origin/main
### an editor will open up, remove the first three lines with unrelated commits, save and exit. Should say "Successfully rebased"
git push -f <your-remote> blockly_enhancements_1

(replace <your-remote> with the name of the remote with your fork)

That is provided those three extra commits don't have anything you need for this PR.

@stefan-hoehn
Copy link
Contributor Author

@asmigala Thanks for helping me - very much appreciated.

I struggle with deletion of the "three lines"... (see at the end)

This is what I do

  • I have cloned my repo again into a new directory to make sure I am not losing anything of what I have developed so far

git clone https://github.com/stefan-hoehn/openhab-webui/ git remote add upstream https://github.com/openhab/openhab-webui.git

  • Then I do as you say

git checkout blockly_enhancements_1 git fetch origin

  • To see where I am at I show the last commits with "tig" (I added the commit hashes)

`
cffa127 2021-10-16 13:02 +0200 Stefan Höhn o [blockly_enhancements_1] {origin/blockly_enhancements_1} [blockly] add new blocklies: audio, v

789b72a 2021-04-10 07:39 +0200 Stefan Höhn o [main] {origin/HEAD} {origin/main} allow width/height for floorplan marker as advanced params

001160b 2021-04-07 14:51 +0200 Stefan Höhn o allow width/height for floorplan marker as advanced params(fix typo)
9c4079c 2021-04-07 11:46 +0200 Stefan Höhn o allow width/height for floorplan marker as advanced params

ebd4b04 2021-04-02 15:52 +0200 Wouter Born o Upgrade Karaf to 4.3.1 (#984)

`

I'd be happy to loose the last four commits as I have a copy of the newest and the other 3 can be lost

Now I do

git rebase -i origin/main
and the editor shows:

pick cffa127 [blockly] add new blocklies: audio, voice, timer

# Rebase 789b72a..cffa127 onto 789b72a (1 command)
#

I have no experience with rebase but reading the manual, doesn't have this to be changed to include the last one ebd4b04 like so?

#not necessarily this one but would be okay
drop cffa127

# but these
drop 789b72a
drop 001160b
drop 9c4079c
pick ebd4b04

or (again, I really don't know what I am doing ;-)

Rebase ebd4b04..cffa127c onto ebd4b04

Thanks again for you help,
Stefan

@asmigala
Copy link

@stefan-hoehn sorry, my bad, I assumed you have the upstream repo as origin and you own for as some other name (that's how I organize it, but I agree it's non-standard)

So the correct steps should be:

git fetch upstream 
git checkout blockly_enhancements_1
git rebase -i upstream/main
### delete the three commits
git push -f origin blockly_enhancements_1

Now to get you main branch in line with the upstream main, you can do

git fetch upstream
git checkout main
git reset --hard upstream/main

And that will just make your main match whatever the upstream main is (discarding any changes you might have in your main)

Signed-off-by: Stefan Höhn <stefan@andreaundstefanhoehn.de>
Signed-off-by: Stefan Höhn <stefan@andreaundstefanhoehn.de>
…imer, ephemeris

Signed-off-by: Stefan Höhn <stefan@andreaundstefanhoehn.de>
Signed-off-by: Stefan Höhn <stefan@andreaundstefanhoehn.de>
Signed-off-by: Stefan Höhn <stefan@andreaundstefanhoehn.de>
@relativeci
Copy link

relativeci bot commented Oct 20, 2021

Job #243: Bundle Size — 10.73MB (+0.28%).

900ee5c vs 0606df3

Changed metrics (3/8)
Metric Current Baseline
Cache Invalidation 10.78% 0.76%
Modules 1526(+0.07%) 1525
Packages 131(+0.77%) 130
Changed assets by type (1/7)
            Current     Baseline
JS 8.46MB (+0.36%) 8.43MB

View Job #243 report on app.relative-ci.com

Signed-off-by: Stefan Höhn <stefan@andreaundstefanhoehn.de>
@stefan-hoehn
Copy link
Contributor Author

A comprehensive description can be found at https://community.openhab.org/t/extending-blockly-with-new-openhab-commands/127169/45

@asmigala Thank you so much for the git support!

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

Here is a first round of review. :)

General remarks:

  • all blocks should have the same orangeish color identifying them as from the "openHAB" category (hue: 0).
  • all blocks labels should read like a natural English sentence.

@stefan-hoehn
Copy link
Contributor Author

Signed-off-by: Stefan Höhn <stefan@andreaundstefanhoehn.de>
Signed-off-by: Stefan Höhn <stefan@andreaundstefanhoehn.de>
Signed-off-by: Stefan Höhn <stefan@andreaundstefanhoehn.de>
@stefan-hoehn
Copy link
Contributor Author

stefan-hoehn commented Oct 24, 2021

@asmigala Can I ask you again one thing? The check noticed that I forget a signoff on particular commit (#1179 cd92dfc). Can you help me how I can only amend that one commit to have a --signoff as well?

See https://github.com/openhab/openhab-webui/pull/1179/checks

@asmigala
Copy link

asmigala commented Oct 24, 2021

@stefan-hoehn the easiest way is probably using interactive rebase again, so git rebase -i upstream/main, in the editor, find the line with the commit and change the first word from pick to reword, save and exit, a new editor will open with that commit's message and you can add the sign-off line manually.

Btw I see your PR has 15 commits now, including a merge commit (which is usually a no-no), so you might want to ask the maintainers of this repo whether you should squash.

@ghys
Copy link
Member

ghys commented Oct 25, 2021

If there's one sign off missing on a single commit in a multiple-commits PRin but all the rest are, and if it's pretty clear that it was a mistake and not a willful omission for that particular commit, then I would say it's not such a big deal. On merge these commits will be squashed with the sign-off line anyways. However you should have added @bigbasec's sign-off as a Also-by: line since you took over their code. I'll make sure of it as well for the final commit.

To avoid forgetting signoff, if you use VS Code, there is an option: "Git: Always Sign Off" (you can find it by searching).

Signed-off-by: Stefan Höhn <stefan@andreaundstefanhoehn.de>
@stefan-hoehn
Copy link
Contributor Author

This should be hopefully the finally push for the enhancement. Believe me, I tried several times to rebase and squash the commits but failed badly (I pay someone a 🍺 if s/he is explaining me how to do this right 😢 ).

So @ghys, can you please see if this all can now be merged? Thanks

@bigbasec
Copy link
Contributor

bigbasec commented Nov 1, 2021

Not sure that this is even still all valid and working. I worked on this a looong time ago, but never got any feedback or anything, so I sort of dropped it. It's not something I ever intended to use anyway. I can probably still help here a bit, but not sure what needs to be done still.

@stefan-hoehn
Copy link
Contributor Author

hey @bigbasec - good to see you finally. You provided a really good foundation you can be proud of on which I have based my work on. Without that I wouldn't have gotten that far so quickly. I refactored, fixed a lot, made it work, extended it and added quite some code and tested it. Now I hope we can release it very soon and everyone can use it. Nothing to do here at the moment anymore but you really deserve the appreciation to have it all started. If you like to see what happened in the last weeks, you can look here.

@ghys
Copy link
Member

ghys commented Nov 1, 2021

Just so you know - I'll make sure I will credit you both equally in the final commit as you've both made sizeable contributions.

@stefan-hoehn
Copy link
Contributor Author

@ghys Just to be sure (and not meaning to nag you) - Do you need anything from me to merge the PR? I am done with my work so far.

Signed-off-by: Yannick Schaus <github@schaus.net>
@relativeci
Copy link

relativeci bot commented Nov 7, 2021

Job #250: Bundle Size — 10.74MB (~-0.01%).

2ef2225 vs e18c5c4

Changed metrics (1/8)
Metric Current Baseline
Cache Invalidation 0.57% 6.85%
Changed assets by type (1/7)
            Current     Baseline
JS 8.47MB (~-0.01%) 8.47MB

View Job #250 report on app.relative-ci.com

Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

Okay so I finished this myself, these last commits include a lot of adjustments & refactoring so make sure you pull the latest changes from main and start from there if you want to continue adding more blocks.

Also updated to the latest stable Blockly (Q2 2021, 6.20210701.0) - see https://github.com/google/blockly/releases for the changelog.
Noticed the upcoming release has a lot of changes, including the ability to define the toolbox using JSON instead of XML, which looks interesting to simplify the dynamic creation with the (upcoming) user-defined Blockly libraries.

@ghys ghys added enhancement New feature or request main ui Main UI labels Nov 7, 2021
@ghys ghys added this to the 3.2 milestone Nov 7, 2021
@ghys ghys merged commit 074b3f2 into openhab:main Nov 7, 2021
@ghys ghys changed the title Blockly enhancements 1 - Expand Blockly with new blocks (part 1) & update to latest version Nov 7, 2021
@ghys ghys mentioned this pull request Nov 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants