-
Notifications
You must be signed in to change notification settings - Fork 78
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
#2178 new objects only after reload #2181
#2178 new objects only after reload #2181
Conversation
Thanks a lot I will check tomorrow. Still confusing that it was broken by the export objects PR. Habe you tried if it just magically works without the loadash import? Because all other logic will never be called which was added in the PR without using the buttons. Of course not an option but I am curious, but of course I can try it myself tomorrow 😬 |
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.
Thanks a lot for the PR! Makes a lot of sense and looks very good to me.
I just proposed a small change in the if
logic and would not add code that is not necessary for the fix.
Let me know what you think.
@@ -2371,9 +2371,9 @@ class ObjectBrowser extends Component { | |||
async componentDidMount() { | |||
await this.loadAllObjects(!objectsAlreadyLoaded); | |||
if (this.props.objectsWorker) { | |||
this.props.objectsWorker.registerHandler(this.onObjectChange); | |||
this.props.objectsWorker.registerHandler(this.onObjectChange.bind(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.
Probably not necessary for the fix, if not needed I would leave it out personally.
src/src/components/ObjectBrowser.jsx
Outdated
@@ -3007,6 +3007,8 @@ class ObjectBrowser extends Component { | |||
} else { | |||
delete this.objects[event.id]; | |||
} | |||
} else if (event.obj) { |
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.
I personally would prefer, remove line 3004 and put in 3007 else if (this.objects[event.id])
From my perspective this would unwire the code a bit
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.
Should be resolved by new approach.
Previously I tried to keep the change minimal to reduce workload for review, but of course the new variant is cleaner
src/src/components/ObjectBrowser.jsx
Outdated
@@ -3031,6 +3033,8 @@ class ObjectBrowser extends Component { | |||
} else { | |||
delete this.objects[id]; | |||
} | |||
} else if (obj) { |
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.
would follow the same recommendation as above
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.
as stated above
And please add a short description of the fix at https://github.com/ioBroker/ioBroker.admin/blob/master/README.md#work-in-progress |
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 good, I like the refactoring 👍
Hello @foxriver76,
please review these changes carefully, which should hopefully resolve #2178.
In my local test enviroment, I was able to add new objects. Personally I still can't explain why this should have worked in 6.0.11 and stopped working in 6.0.12.
Somehow
this.objects
must have gotten the newly added object outside ofonObjectChange
in the past but I couldn't pinpoint that.Best regards
Thiemo