Opened 4 years ago

Closed 4 years ago

#2081 closed feature request (done)

New HL data types for dealing with files and paths

Reported by: mkommend Owned by: mkommend
Priority: medium Milestone: HeuristicLab 3.3.9
Component: Data Version: 3.3.9
Keywords: Cc:

Description

External evaluation problems (Matlab, Scilab) often rely on existing scripts which should be executed to calculate a quality value. Instead of saving the paths to the scripts are saved as strings, new HL data types should be created, that allow more meaningful parameters and a specialized GUI (e.g., File open to choose a script, preview, ...).

Change History (30)

comment:1 Changed 4 years ago by mkommend

  • Status changed from new to accepted

comment:2 Changed 4 years ago by mkommend

r9667: Branched HeuristicLab.Data.
r9668: Corrected Branch of HeuristicLab.Data.
r9669: Branched HeuristicLab.Data.Views.
r9670: Removed wrongly branched directory 3.3.

comment:3 Changed 4 years ago by mkommend

  • Owner changed from mkommend to ascheibe
  • Status changed from accepted to reviewing

r9671: Added new path data types and view.

comment:4 Changed 4 years ago by mkommend

  • Milestone changed from HeuristicLab 3.3.x Backlog to HeuristicLab 3.3.9

comment:5 Changed 4 years ago by mkommend

r9680: Corrected namespaces of new path value types and added build command.

comment:6 Changed 4 years ago by mkommend

r9686: Corrected plugin dependencies of HeuristicLab.Data.Views.

comment:7 Changed 4 years ago by mkommend

r9689: Corrected references to copy local = false in HL.Data.*.

comment:8 Changed 4 years ago by ascheibe

  • Owner changed from ascheibe to mkommend
  • Status changed from reviewing to assigned

I have reviewed the changes:

  • PathValue: When setting the Value property, an exception may be thrown that talks about 'file'. As this could also be a directory, it would be better to call it e.g. 'path' instead of 'file'.
  • When a TextFileView is displayed and the the ViewHost is used to switch to the FileValueView, a NullReferenceException is thrown.
  • If a file is loaded in the TextFileView and you switch to FileValueView, the following exception is thrown: System.ArgumentException: The directory name is invalid.
  • The textboxes of file and directory values look different. The directory textbox is 3d while the file textbox is flat.
  • In the TextFileView, there are 2 lines that say: TODO handle IO Exceptions
  • openButton_Click works differently (code-wise) in the DirectoryValueView and the FileValueView. Shouldn't this be the same as both views do approximately the same thing?
  • TextFileView, line 70: I think Content.Exists() would be more appropriate than File.Exists(Content.Value)
  • As we have discussed, CheckExistence does not seem to make a lot of sense as it is never really used as far as I can see. Also, the TextFileView implements it's own logic for checking this, so it should be removed. On the other hand, if you allow for relative path and editing of the textboxes with the paths, then it would make sense again.

comment:9 Changed 4 years ago by mkommend

  • Owner changed from mkommend to ascheibe
  • Status changed from assigned to reviewing

r9697: Updated branches from trunk and implemented all review comments.

comment:10 Changed 4 years ago by mkommend

r9704: Merged trunk changes into path datatypes branch.

comment:11 Changed 4 years ago by mkommend

  • Owner changed from ascheibe to mkommend
  • Status changed from reviewing to assigned

Found some additional issues during testing which must be resolved.

comment:12 Changed 4 years ago by mkommend

  • Owner changed from mkommend to ascheibe
  • Status changed from assigned to reviewing

r9705: Removed the code for clearing the properties of the filesystemwatcher in the TextFileView as this could lead to exceptions.

comment:13 Changed 4 years ago by ascheibe

  • Owner changed from ascheibe to mkommend
  • Status changed from reviewing to readytorelease

Ok thanks, looks good, I have tested the changes. The only comment that I have is that in r9697 in the HeuristicLab.Data.Views project the reference to HeuristicLab.Data got modified and CopyLocal was set to true. Please be careful when merging as this should not get into the trunk.

comment:14 Changed 4 years ago by mkommend

  • Version changed from branch to 3.3.9

r9714: Integrated new HL path data types from the branch.

comment:15 Changed 4 years ago by mkommend

r9716: Deleted branch for HL path data types.

comment:16 Changed 4 years ago by gkronber

  • Owner changed from mkommend to gkronber
  • Status changed from readytorelease to reviewing

I just discovered these new data types which are seemingly derived from StringData. I'd like to also review this before this ticket is closed.

comment:17 follow-up: Changed 4 years ago by gkronber

  • not quite sure it is reasonable to derive PathValue from StringValue
  • storing file-filter strings for a dialog (GUI specific) in a core data-type is somehow strange.
  • it seems the only reason for implementing these data-types is to save the effort to implement specific controls?

comment:18 in reply to: ↑ 17 Changed 4 years ago by mkommend

Replying to gkronber:

  • not quite sure it is reasonable to derive PathValue from StringValue

Yes it is questionable, if PathValue should be derived from StringValue. However, this can be easily fixed.

  • storing file-filter strings for a dialog (GUI specific) in a core data-type is somehow strange.

AFAIK, this is the only way how to provide and persist GUI specific information in HL. The only improvement would be to store the information in a separate class (similar to the visual properties of a data table), which is a little bit of an overkill for just one property.

  • it seems the only reason for implementing these data-types is to save the effort to implement specific controls?

To provide specific controls a specialized data-type must be implemented. I thought that the functionality may be helpful for others as well and therefore I implemented generic types to handle file and path values.

comment:19 Changed 4 years ago by gkronber

  • Owner changed from gkronber to mkommend
  • Status changed from reviewing to assigned

Please don't derive from StringValue and remove the filter-changed-event.

comment:20 Changed 4 years ago by mkommend

  • Owner changed from mkommend to gkronber
  • Status changed from assigned to reviewing

r9833: Implemented reviewer comments by gkronber for path values.

comment:21 Changed 4 years ago by gkronber

Reviewed r9833.

Tested the file-watcher functionality and found an issue.

  1. create a TextFileValue and select a text file
  2. change the file externally
  3. a dialog will pop up if the file should be reloaded (press ok)
  4. change the file externally again
  5. an exception occurs

comment:22 Changed 4 years ago by gkronber

  • Owner changed from gkronber to mkommend
  • Status changed from reviewing to assigned

comment:23 Changed 4 years ago by mkommend

r9841: Changed FileShare to read / write for file reloading.

comment:24 Changed 4 years ago by mkommend

r9850: Added exception handling to the TextFileView.

comment:25 Changed 4 years ago by mkommend

  • Owner changed from mkommend to gkronber
  • Status changed from assigned to reviewing

Fixed the described error with r9841.

comment:26 Changed 4 years ago by gkronber

  • Owner changed from gkronber to mkommend
  • Status changed from reviewing to assigned

Reviewed r9841 and r9850 and tested the changes.

The FileValueView does not show an "Open file"-button

comment:27 Changed 4 years ago by mkommend

r9949: Corrected location of open button in FileValueView.

comment:28 Changed 4 years ago by mkommend

  • Owner changed from mkommend to gkronber
  • Status changed from assigned to reviewing

comment:29 Changed 4 years ago by gkronber

  • Owner changed from gkronber to mkommend
  • Status changed from reviewing to readytorelease

Reviewed and tested r9949.

comment:30 Changed 4 years ago by mkommend

  • Resolution set to done
  • Status changed from readytorelease to closed

r9961: Merged path data types into stable.

Note: See TracTickets for help on using tickets.