Fix display for nested/vlen cmpds - #474
Conversation
Just an initial few comments; I think that expanding into one column per variable-length sequence element will create far too many columns to be useful in general. I don't think this is generally necessary either, as the tcompound_complex2.h5 file shows that a compound datatype nested 3 levels down inside a very complex top-level compound datatype can still be displayed fairly reasonably.
A variable-length sequence string as a member of that compound should be easy to display in a single column as well. One issue that now makes this difficult is that something changed to make the column headers no longer collapsible or selectable, and instead if r/w is enabled the table allows you to try editing them (which shouldn't be possible). The column headers with arrows used to be collapsible by double-clicking, which made reading complex compound types a bit easier. For editing, one column per sequence element would be convenient, but likely only if the sequence is very short which often isn't the case. The current method of displaying them as a string in brackets isn't particularly nice for editing, but it's been suggested in the past that editing a variable-length sequence could open a separate table for viewing/editing, similar to how object/region reference objects work currently (though those have some issues as well). I also want to mention that display of fairly arbitrarily-nested compound/vlens inside compounds also used to work previously, which makes me think that the fix for displaying these should generally be a simple bugfix for something that changed several releases ago. |
|
The initial implementation on this branch displayed a vlen seq by expanding it into one column per sequence element, producing an unbounded number of columns for long or deeply-nested sequences. Every vlen now renders as a single column showing the whole sequence as a bracketed string, recursing into nested compounds. For example, a VLEN<COMPOUND{a, sub:{p,q}}> member shows [{10, {11, 12}}, {20, {21, 22}}]. For a vlen seq that is a member of a compound, the column-index maps treat it as one column and CompoundDataProvider delegates the cell to VlenDataProvider's whole-sequence rendering instead of fetching one element per column offset. For a top-level vlen-of-compound dataset, the whole sequence is read in one H5DreadVL call, and the datatype is enumerated as a single member, so the dataset dispatches to VlenDataProvider as one column instead of being peeled into per-member columns. This is more in line with HDFView's historic pattern for displaying nested data. |
jhendersonHDF
left a comment
There was a problem hiding this comment.
After reviewing this, I think a decent bit of the changes are mostly unnecessary or obsolete after the fixes made in the JNI. We should retest with an HDF5 2.3.0 build and see where remaining issues are at. The only datatype that seems problematic is a vlen of compound, which was just unsupported in 3.1.1 but at least didn't cause *ERROR* like 3.4.1. Compound of vlen of compound displayed fine in 3.1.1. Even a compound of array of compound of vlen of compound OR a compound of vlen of compound of array of compound displayed fine in 3.1.1. Of course "fine" is up to taste since it was nested braces and brackets and could probably be display better in separate tables, but that's really a different issue from this PR.
| * Show the editing-disabled notice. Called when the user attempts to edit | ||
| * a cell in a dataset whose datatype has no symmetric write path (see | ||
| * {@link DataFactoryUtils#isUnsafeForWrite}). Throttled to one dialog per | ||
| * second so a held-down key or rapid clicks don't stack popups. |
There was a problem hiding this comment.
This shouldn't be necessary; the dialog box opened should be a blocking dialog so the user should have to interact with it first before being able to open another.
| if (dtype == null) | ||
| return false; | ||
|
|
||
| if (dtype.isVLEN() && !dtype.isVarStr()) |
There was a problem hiding this comment.
If I'm reading this correctly it doesn't seem quite right, as HDFView has had limited support for writing variable-length types for a bit now.
| */ | ||
| public static boolean isUnsafeForWrite(Datatype dtype) { return isUnsafe(dtype, false); } | ||
|
|
||
| private static boolean isUnsafe(Datatype dtype, boolean insideCompound) |
There was a problem hiding this comment.
For maintainability this really seems like something that should be handled by the individual DataProviderFactory classes instead of here, since that's where the editing logic will be at. And instead of displaying an information dialog each class should throw an UnsupportedOperationException instead. See https://archive.eclipse.org/nattable/releases/1.1.0/apidocs/org/eclipse/nebula/widgets/nattable/data/IDataProvider.html#setDataValue(int,%20int,%20java.lang.Object)
| // unexpanded member count. Editing is already disabled for | ||
| // these via unsafeForWrite, and the listener's side effects | ||
| // (ref preview, etc.) don't apply, so just return. | ||
| if (unsafeForWrite) |
There was a problem hiding this comment.
I'm not sure I understand returning here, because this is an event that fires for updating the cell value textarea when selecting a cell. It shouldn't have anything to do with whether a value is safe for writing.
| if (baseType != null && baseType.isCompound()) { | ||
| theData = compoundTypeIO(ioType, did, spaceIDs, nSelPoints, (H5Datatype)baseType, writeBuf, | ||
| globalMemberIndex); | ||
| if (ioType == H5File.IO_TYPE.READ) { |
There was a problem hiding this comment.
This change doesn't make sense to me - it seems to just throw out the writing case entirely
| if (dsDatatype.isVarStr()) { | ||
| if (dsDatatype.isVarStr() || | ||
| (dsDatatype.isArray() && dsDatatype.getDatatypeBase() != null && | ||
| dsDatatype.getDatatypeBase().isVarStr())) { |
There was a problem hiding this comment.
I don't think this should be needed; Arrays of variable-length strings displayed fine in HDFView 3.1.1. They cause a crash in 3.4.1, but that's probably due to the fixes that were needed in the JNI.
|
|
||
| if (dsDatatype.isVarStr()) { | ||
| if (dsDatatype.isVarStr() || | ||
| (dsDatatype.isArray() && dsDatatype.getDatatypeBase() != null && |

Currently, attempting to open a compound dataset with members containing vlen sequences, or compounds nested inside of vlen sequences, leads to missing data, missing column headers, or failure to open the dataset entirely.
This set of changes introduces a new standard for the display of vlen sequence data, and resolves several related bugs in vlen sequence/compound type handling.
The new practice for variable sequences is to expand into one column per sequence element. Empty sequences expand into a single empty placeholder column. This is a change from #472, which handled variable length sequences by displaying them as strings.
This is more complex and may inhibit readability for dataset with long sequences. However, something of this form is necessary for datasets with complex nested types to be viewable at all within HDFView, so the tradeoff is judged to be worthwhile.
filterNonSelectedMembersremoves unselected member fields from a compound by checking against the dataset's flat selected-member list. This list only enumerates the dset's top-level leaf members. When called on an inner compound reached via recursion (e.g. when a cmpd was the base type of a vlen sequence), the member fields did not appear in the flat list, and so the inner compound came back empty. Resolved by the introduction of theisTopLevelparameter, which can be specified as false to skip the filter and preserve the inner compound's members.recursiveColumnHeaderSetupwalks the dataset's flat list of leaf names and looks up the corresponding top-level member type for each to decide how to render the column. It did this by retrieving the n-th member, where n was current index modulo the size of the member types. This calculation only made sense when each top-level meber had one leaf, and would produce incorrect results for heterogenous shapes. I replaced the calculation with explicit topIdx tracking via a newcountLeafNameshelper.In H5CompoundDS, a vlen member is read with H5DreadVL using a single-field compound transfer type. The JNI returns each row wrapped in a one-element record, so memberData[r] was an ArrayList containing the data rather than the data itself. This is now unwrapped to expose the data directly.
These changes depend on changes to HDF5's JNI VL reading in HDFGroup/hdf5#6413, and so these won't be able to be merged into the master branch until hdf5#6413 is in a new version of the HDF5 library, and HDFView can be bumped to use HDF5 2.2.0 instead of HDF5 2.0.0.
Resolves #470