iTop
iTop copied to clipboard
Combodo
Display binary data size in SynchroReplica details
Currently the SynchroReplica details page shows all data from the synchro_data table as unformatted/raw. With this change it doesn't try to display binary data directly anymore, but it now shows the size of the data.
Before:
After:
Disclaimer: The sample data is not an actual image, but just 128 sample bytes.
Hello, Do you need to see the base64 content ? Or should we replace it with the mention "blob" and value size maybe ?
No, but with it being base64 encoded, the user could copy and download/convert the data easier. But I like your suggestion as well.. Hmm 🤔
@piRGoif @Molkobain Do you have any update? What route should we take? Show content, show meta (size), show both?
Hello, Sorry I moved this PR to the "pending contributor" state and it was a bad idea... I'll change the state again so that this PR will be processed on next reviews !
Reviewed during PR technical review : actually the main problem is that we are getting raw data and display them as is... This will create problems also for HTML attributes for example... We don't have any helper method for now that could help displaying raw values from a MySQL table (datasynchro data table)...
Can you also add a \utils::EscapeHtml call on value for attributes other than AttributeBlob ? We all agreed displaying base64 data won't be very useful. What can be displayed instead ? I suggested the size, but there might be other metadata brought from the DB ? Having a download link would be ideal but that would mean create a new endpoint...
Thanks Thomas !
Would you also replace the base64 value by something else, maybe the size ? To display the value properly you could use \SetupUtils::HumanReadableSize
I have done the change, but I feel like we still need to show the first x bytes (encoded)
I have done the change
Thanks !
I feel like we still need to show the first x bytes (encoded)
Can you give some use cases where it would be useful ? We didn't find any when discussing about the feature ?
Yeah, what is the use case of this whole feature.. It gives the ability for the user to see if the data makes any sense or not.
But if you all agree that only showing something as follows is good enough, then that's fine by me.
234 kB
:D I understand ! There's no doubt this is far from ideal now, and still after this PR. But your PR improves things a bit, for us it's better now ! The PR will be reviewed again next week.
Maybe it should say something like this instead of only the size?
Binary data (234kB)
or
BLOB (234kB)
Very nice idea, OK for me ! I would maybe prefer "binary data", as "blob" is more of an implementation detail ? Can you change it before the review on thursday ?
I would have used the dictionary for Core:AttributeBlob. But anyway, applied the requested change.
Oh, I didn't knew Core:AttributeBlob : you're right, it's better to use it ! Sorry :/
Can you do this last change ?
It was also you that suggested to use "blob" anyway 😛
Hello, Do you need to see the base64 content ? Or should we replace it with the mention "blob" and value size maybe ?
Hello Thomas, next review in a few weeks, do you mind updating the picture in the OP to reflect the new result? It will ease convincing. Thanks!
(Nevermind the notification, I thought I would be able to commit it directly myself. I'll do it aftermerge)
Didn't got any notifications. If you still have suggestions, you can add them here..
Hello Thomas,
This was accepted in functional review. Exceptionally as the rest of the feature (#269) was merged in support/3.0 we would like to merge this one in support/3.0 as well, can you rebase the PR?
Thanks, Guillaume
These two are not dependant on each other.. It was already possible to sync binary data by directly connecting the datasynchro tables..
These two are not dependant on each other.. It was already possible to sync binary data by directly connecting the datasynchro tables..
Well, I think Guillaume meant that we would like to group those 2 blob sync fixes in the same release :) Are you ok to rebase this pr branch ?
Indeed, but it's too late anyway, 3.0.2 is on code freeze. Having it on the support/3.0 should a good though.
What is the status on this PR, I see it's on "pending contributor update". Did we just wanted to rebase on support/3.0 so we could merge it in the 3.0.2? If so, that train departed a long time ago, we could merge it in support/3.0 any time now.
@piRGoif @Hipska any memories?
I see in conversation history it was requested to base on 3.0, which I did.
Oh, but also seem to have reverted.
Hello, Indeed we asked to rebase the branch on support/3.0. Currently the branch start is 42599eae which is in develop... Can you do the rebase Thomas ?