search

BHoM / BHoM_UI

GNU Lesser General Public License v3.0
9 stars 6 forks source link

Provide a report of the upgraded component when opening a script #390

Closed adecler closed 3 years ago

adecler commented 3 years ago

NOTE: Depends on

https://github.com/BHoM/Versioning_Toolkit/pull/159 https://github.com/BHoM/BHoM/pull/1237 https://github.com/BHoM/BHoM_Engine/pull/2495

Issues addressed by this PR

Closes #389

When a script is opened, if any component was upgraded through teh versioning toolkit, a dialog would pop-up with the details regarding the upgrades done to the script. Here's what it looks like:

image

Note that I've organised the data that way to facilitate the comparison of the old and new versions as it is sometimes tricky to spot what changed.

Test files

https://burohappold.sharepoint.com/:f:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/BHoM_UI/%23389-VersioningReport?csf=1&web=1&e=YMt3tB The report above was generated using the Excel file from that folder. The hidden BHoM Sheet is a convenient way to test specific outdated items without the need to have a file containing that specific component. Best to test on other outdated scripts as well though.

adecler commented 3 years ago

Thanks @FraserGreenroyd ,

adecler commented 3 years ago

@BHoMBot check core @BHoMBot check copyright-compliance @BHoMBot check versioning

bhombot-ci[bot] commented 3 years ago
@adecler to confirm, the following checks are now queued: - `core` - `copyright-compliance` - `versioning` There are 6 requests in the queue ahead of you.
FraserGreenroyd commented 3 years ago

Have re-reviewed following the changes and have the following comments:

  • Copy text: Ha! the simple things are sometimes the more problematic. WinFrom labels do not support selection. TextBoxs do but they are not so great with the AutoSize. So you end up having to manually calculate the size of the box. This becomes problematic for those that have to wrap words on multiple lines. Funnily enough, dealing with web links is easier (at least assuming there is only one link). So I have added support for that.

This worked well and made it easy enough to do the crucial action where links are involved. I can imagine possible scenarios where multiple links are included, but I think we can include support for that at a later date, happy for that to be done as a small injection.

  • Making it clear this is just for the BHoM: Added icon and more specific text.

Think this works well now with the logo and additional use of BHoM in the description.

  • Same description for old and new: YEs, that one is more tricky. Because a type is only described by its namespace + name. And I don't want to start adding things related to the input/outputs of a component inside the versioning engine to avoid having engine depending on UI. I could add some generic thing like (properties changed) if you want.

I feel a generic message along the lines of the properties of this object have been updated. would be beneficial to help highlight what the versioning change is - but am happy for that to be a later injection to avoid too much work to this PR scope? Will let you judge that.

  • Window not closing on Excel: That is weird! it closes fine for me. IT might be related to Excel sill not closing properly sometimes. Can you try a few more times to make sure. If it keeps happening, we should do a screen share.

As I tried again I got this. Long time no see... not sure if it's linked to this PR, or Excel generally as it has been a very long time since I used BHoM in Excel - I am also happy for this to not hold up this PR as this may be machine specific, or out of scope of what this PR is introducing. The report itself renders fine, and I'm sure the odds of someone closing Excel before closing the versioning report are slim (though admittedly not zero).

image

If you're happy @adecler I'm happy to approve all the PRs in this series with an aim to merge in and get some alpha testing done in the next few weeks - I'll let you decide regarding the message for objects with properties as to whether you want to add to this PR or raise an issue for later. Same with multiple links. If you opt to go for merge now, if you want to handle any outstanding check requests (I won't do it now in case you decide to push commits, just to save the computing power :smile: ) and I can approve/merge in the morning (my time). Alternatively if you opt to push some updates I'll run another round of tests and then approve/merge as appropriate :smile:

adecler commented 3 years ago

Thanks @FraserGreenroyd ,

I have added the generic message on objects that have updated properties. I have also handled the issue of Excel crashing when closing by managing the thread explicitly when a workbook is closed. It looks like Excel was a bit more picky than Grasshopper on that regard.

adecler commented 3 years ago

@BHomBot check required

bhombot-ci[bot] commented 3 years ago
@adecler to confirm, the following checks are now queued: - `code-compliance` - `documentation-compliance` - `project-compliance` - `core` - `null-handling` - `serialisation` - `installer` - `versioning`
bhombot-ci[bot] commented 3 years ago
@adecler just to let you know, I have provided a `check-versioning` result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @adecler on BHoM
bhombot-ci[bot] commented 3 years ago
FAO: @FraserGreenroyd @adecler is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate. The check they wish to have dispensation on is project-compliance. If you are providing dispensation on this occasion, please reply with: > @BHoMBot this is a CI/CD instruction. I am authorising dispensation to be granted on check ref. `2572415398`
adecler commented 3 years ago

@FraserGreenroyd , it looks like the bot is confused between Versioning engine and the Version keyword in dll references.

adecler commented 3 years ago

@BHoMBot check copyright-compliance @BHoMBot check core @BHoMBot check versioning @BHoMBot check installer

bhombot-ci[bot] commented 3 years ago
@adecler to confirm, the following checks are now queued: - `copyright-compliance` - `versioning` - `installer`
adecler commented 3 years ago

@BHoMBot check copyright-compliance @BHoMBot check core @BHoMBot check versioning @BHoMBot check installer

bhombot-ci[bot] commented 3 years ago
@adecler to confirm, the following checks are now queued: - `copyright-compliance` - `core` - `versioning` - `installer`
bhombot-ci[bot] commented 3 years ago
@adecler just to let you know, I have provided a `check-versioning` result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @adecler on BHoM_Engine
bhombot-ci[bot] commented 3 years ago
@adecler just to let you know, I have provided a `check-versioning` result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @adecler on BHoM
FraserGreenroyd commented 3 years ago

Thanks @FraserGreenroyd ,

I have added the generic message on objects that have updated properties. I have also handled the issue of Excel crashing when closing by managing the thread explicitly when a workbook is closed. It looks like Excel was a bit more picky than Grasshopper on that regard.

Have seen the generic message and am happy with it.

Excel also closed nicely instead of having a hissy-fit :smile:

I have followed the same test I outlined in my first review and am happy with the outcome, and cannot find any more things to improve at this stage (I reserve the right to find things next week 😝 ). As such, I'm approving this PR and all PRs in the series.

I note that we're not waiting for many more reviews based on the reviewer list across the PR series, so I'll merge these in now so that we get an alpha tomorrow with this included for others to take a look at via the alpha artefacts.

@adecler I'll leave to you to post some comms about this addition on Teams/Yammer/Slack as appropriate if you wish. We should probably look to do a review session on this Engine PR as well shortly before we get too close to beta.