Opened 10 years ago

Closed 10 years ago

#1338 closed enhancement (done)

DataTable should be unsealed

Reported by: epitzer Owned by: swagner
Priority: medium Milestone: HeuristicLab 3.3.3
Component: Analysis Version: 3.3.3
Keywords: Cc:

Description (last modified by epitzer)

The DataTable class has a lot of potential to be extended, customized or marked through deriving a custom data type. Therefore, it should be unsealed.

Change History (13)

comment:1 Changed 10 years ago by epitzer

  • Description modified (diff)
  • Owner changed from swagner to epitzer
  • Status changed from new to accepted

comment:2 Changed 10 years ago by epitzer

DataTable, DataRow and DataTableView have been unsealed. (r5097)

A possible persistence problem has also been fixed by directly persisting the visualProperties property otherwise an ArgumentException might break backwards compatibility.

comment:3 Changed 10 years ago by epitzer

  • Owner changed from epitzer to swagner
  • Status changed from accepted to reviewing

comment:4 Changed 10 years ago by swagner

Thanks. The changes for unsealing DataTable, DataRow and DataTableView look good.

However, I do not understand the persistence problem regarding the visual properties. As far as I understand, the ArgumentException cannot occur as the visual properties have never been and will never be persisted as a null value. The after deserialization hook ensures that a DataTable or DataRow always has a valid visual properties instance. Therefore I think that backwards compatibility is not in danger. But probably I miss something? May you explain your concerns in more detail? Thanks.

comment:5 Changed 10 years ago by swagner

  • Owner changed from swagner to epitzer

comment:6 Changed 10 years ago by epitzer

  • Owner changed from epitzer to swagner

@swagner: You are right. The visual properties would have to have been persistence as null value which is very unlikely. However, I still think that directly persisting the field instead of going through the accessor is cleaner and faster as no code for not-yet-registered events has to be run.

comment:7 Changed 10 years ago by swagner

I see. We still have a problem with the solution proposed in r5097 though, as the VisualProperties_PropertyChanged event handler is not properly registered after deserialization of a DataTable or DataRow, if we persist the field. In my opinion there are two alternatives: We can either use an after deserialization hook for registering this event, or we use a persistence property (as it was done before r5097) but access the field directly in this property. I prefer the second solution, as using a hook is not the best idea concerning performance and accessing the field directly in a persistence property also avoids unnecessary events and the ArgumentNullException.

comment:8 Changed 10 years ago by swagner

  • Status changed from reviewing to assigned

comment:9 Changed 10 years ago by swagner

  • Status changed from assigned to accepted

comment:10 Changed 10 years ago by swagner

Added persistence property for visual properties in DataTable and DataRow again in r5261.

comment:11 Changed 10 years ago by swagner

  • Owner changed from swagner to epitzer
  • Status changed from accepted to reviewing

comment:12 Changed 10 years ago by epitzer

  • Owner changed from epitzer to swagner
  • Status changed from reviewing to readytorelease

Very good solution, thanks!

comment:13 Changed 10 years ago by mkommend

  • Resolution set to done
  • Status changed from readytorelease to closed
  • Version changed from 3.3.2 to 3.3.3
Note: See TracTickets for help on using tickets.