[hdpowerview] Refactor dynamic channels

[jlaur]

Signed-off-by: Jacob Laursen [email protected]

Fixes #11852

Refactoring after:

• [hdpowerview] Add support for scene groups #11534
• [hdpowerview] Add support for enabling/disabling automations #11637

Changes

• Extracted creation of dynamic channels into separate classes.
• Renamed methods to be consistent with channel terminology (scene collections -> scene groups, scheduled events -> automations).
• Optimized AutomationChannelBuilder scene/scene collection lookups.
• Reduced log level from error to warning for scheduled events with unknown scene or scene collection.
• Added unit test coverage for SceneChannelBuilder, SceneGroupChannelBuilder and AutomationChannelBuilder.
• Fix small problem with empty properties during startup.

Test

I have been running this version for a longer period in production without any issues. All my channels (scenes, scene groups and automations) are working exactly as before the changes. This is the intention as the PR is purely refactoring not changing any behavior.

[jlaur]

I have created #11857 with a direct bugfix after the findings documented in #11856.

[jlaur]

JAR for this PR.

[jlaur]

JAR: https://drive.google.com/file/d/1rYCHiZu_jn0rP8nkS93PQrxG84BuMpsq/view?usp=sharing

[andrewfg]

As mentioned in my email, I suggest to add the Hunter Douglas API pdf and your API addendum pdf files to the binding's doc folder and also add a 'Developers' chapter at the bottom of the Readme.md with links that point to those files.

[jlaur]

As mentioned in my email, I suggest to add the Hunter Douglas API pdf and your API addendum pdf files to the binding's doc folder and also add a 'Developers' chapter at the bottom of the Readme.md with links that point to those files.

Thanks for your mail, I'm currently responding. Short version here: PDF will not be included in context of this PR at least.

[andrewfg]

I could not test the functionality because I don’t have automations, or scene groups, but the code looks good to me.

[jlaur]

I could not test the functionality because I don’t have automations, or scene groups, but the code looks good to me.

Thanks for having a look, @andrewfg.

[jlaur]

There is now a conflicting file to resolve.

That was expected. It's resolved now.

I reviewed all the code except the tests and I have no remark. I will review the tests later.

Thanks a lot!

[lolodomo]

The problem with the way you fixed the conflict is either I have to do a full review again :-(

[jlaur]

The problem with the way you fixed the conflict is either I have to do a full review again :-(

I'm really sorry about that, what happened and how can I avoid it in the future? From previous PR's I had an understanding that merging back from main is not good practice, but rebase is preferred. Is that correct? So I rebased towards main and resolved the conflicts. All commits were preserved in order to keep same history.

If everything was reviewed excepts tests, why would you have to do a full review again? Conflict was resolved in first commit, which you of course cannot see. Two methods conflicted because they had new throws in other PR and were renamed in this PR. So I think that's the only change which is not transparent because of the rebase history manipulation, but that's kind of natural because it was a rebase.

[andrewfg]

how can I avoid it in the future?

I think the normal process is to hand edit any file that has a merge conflict, to remove the conflict, and then just do a normal commit and push.

[jlaur]

I think the normal process is to hand edit any file that has a merge conflict, to remove the conflict, and then just do a normal commit and push.

But you still need to merge one way or the other to resolve the conflict. So either merge from main or rebase to main.

[jlaur]

I think the normal process is to hand edit any file that has a merge conflict, to remove the conflict, and then just do a normal commit and push.

But you still need to merge one way or the other to resolve the conflict. So either merge from main or rebase to main.

#8902 (comment)

This was what I recalled.

[andrewfg]

This was what I recalled.

Aha. I think it depends on the circumstances of the conflict. I think that there are basically two cases where merge conflicts can arise -- namely..

1. Two people have independently pushed commits to the same java file within the scope of the binding that is subject of the current pull request. In that case it is only necessary to manually edit the respective file that has the conflict (to essentially include the work of both the people concerned), and then re-commit and push that specific file.
2. The branch your-name/openhab-addons/your-branch where the PR is hosted, has become generally out of synchronization with the branch openhab/openhab-addons/main. In this case, the rebase procedure described in kai's link is required.

[jlaur]

I think neither of those cases applied here. It wasn't "generally out of synchronization", but for two specific method signatures there was a change in each PR - method name in one and thrown exceptions in another. If I had manually picked from the other, it wouldn't compile, as my PR didn't have the new exception type. So I had to actually merge, either by rebasing or by merging main back into my branch. @lolodomo - please advice. I would prefer to not obstruct any ongoing PR reviews, but I'm not sure how in this case. I'm pretty annoyed myself when someone will be doing frequent rebases + squashing everything into a single commit, since it makes it impossible to track what changed during the review process.

[lolodomo]

I am not at all a Git expert. The way I am doing it generally is to update my local main branch from the remote main branch and then merge my local main branch into my local work branch, and finally push my local work branch into my remote work branch. It avoids to have all the history pushed again. But in practice, I believe it does not help to identify what has changed with the merge. At least, you know that the only change since the last review was this merge.
If you tell me you have done no other changes except the merge, I believe you. I will just take 2 minutes to do a very very partial look to what I have already reviewed and then switch to the tests.

[jlaur]

The way I am doing it generally is to update my local main branch from the remote main branch and then merge my local main branch into my local work branch, and finally push my local work branch into my remote work branch. It avoids to have all the history pushed again.

Correct, this gives the most detailed change tracking as history will not be manipulated and no force push is needed. However, it also makes history somewhat more complex, i.e. less clean as end result. This was my exact approach in the referenced PR when Kai requested me to rebase instead.

But in practice, I believe it does not help to identify what has changed with the merge. At least, you know that the only change since the last review was this merge.
If you tell me you have done no other changes except the merge, I believe you. I will just take 2 minutes to do a very very partial look to what I have already reviewed and then switch to the tests.

These were the only changes, i.e. conflict resolving:

Other (first exception changed, method still not renamed):

    private List<SceneCollection> updateSceneCollectionChannels()
            throws HubInvalidResponseException, HubProcessingException, HubMaintenanceException {

This (first exception still not changed, method renamed):

    private List<SceneCollection> updateSceneGroupChannels()
            throws JsonParseException, HubProcessingException, HubMaintenanceException {

The same for one other method which was renamed here and had exception changed in other.

So to sum things up, I think I did was I needed to do, even though it somehow interfered with your review?

[lolodomo]

LGTM