Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display sample title instead of filename in DataExplorer window (ESS_GUI) #1652

Merged
merged 3 commits into from
Aug 18, 2020

Conversation

krzywon
Copy link
Contributor

@krzywon krzywon commented Aug 5, 2020

The sample title is displayed in the data explorer instead of the file name. This is useful for distinguishing multiple data sets that are loaded from a single file.

This is the 5th PR separating #1559. I do not plan to create a parallel PR for v4.x/wxPython/master.

Closes #1243.

Copy link
Contributor

@pkienzle pkienzle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is suspicious to see filename used on one side of the assignment and name used on the other, especially since data and fp have both as attributes. Similarly for filenames as a list of name entries.

Please clean up the local variable names so that they reflect their usage. E.g. call it label instead of name or filename if that helps avert confusion.

Furthermore, if you find that you need to assign x.name = y.filename or x.filename = y.name when x and y both have name and filename, please explain with a comment why the sense is changing (or make up a label attribute if that's what you are using it for).

else:
if self.data_is_loaded:
data_ids = [str(self.logic.data.id)]
filenames = [str(self.logic.data.filename)]
filenames = [str(self.logic.data.name)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for data.name to be something other than a string? If it is None, do you want the filename list to be ["None"]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If self.data_is_loaded is True, the name cannot be None, but it could some form of numeral.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems awkward. Can you convert the numeral to a string when you assign it to data.name so you don't have to worry about it elsewhere in the code? That is, how many of your other uses of data.name are implicitly assuming that it is a string, but just haven't been tested yet in the GUI? Are there any bits of the code that use it as a numeral if it is one?

Copy link
Contributor

@pkienzle pkienzle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine. Haven't yet tested.

@pkienzle
Copy link
Contributor

pkienzle commented Aug 7, 2020

Label is displayed when available (for example, from 1d_data/saxess_example.pdh). Names are tagged with [1] when they are duplicated.

@rozyczko
Copy link
Member

rozyczko commented Aug 12, 2020

Code is fine. The only discrepancy in naming shows up on charts and in the reporting widget, where the file name is shown.
Is this by design?

Untitled

@krzywon
Copy link
Contributor Author

krzywon commented Aug 12, 2020

Good catch, @rozyczko. I'll fix the plotters, but need to look more into the Report generation - the filename night be by design.

@krzywon
Copy link
Contributor Author

krzywon commented Aug 13, 2020

I fixed all the issues, including the report generation. The filename is already included in the report, so I changed it for consistency.

@butlerpd butlerpd merged commit c11045a into ESS_GUI Aug 18, 2020
@krzywon krzywon deleted the ESS_GUI_display_name branch August 18, 2020 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants