You could start here for details about how the file should be formatted:
https://wiki.transvar.org/display/igbman/Personal+Synonyms

You might also look a the release test module. It has a functional chromosome.txt file.
That would also give you an idea of how to functionally test if a chromosome file took effect.
https://wiki.transvar.org/display/ITD/Synonyms

The top level of http://igbquickload.org/ has an example of each.
That chromosme.txt file looks oddly short, but I think the synonyms.txt file is a good example.

Thanks Ivory Blakley

Implemented a fix which displays user an error panel on uploading invalid version synonym or chromosome file.
It only checks the valid format and not the validity of data as of now.
Work on the branch : https://bitbucket.org/swathark/integrated-genome-browser/branch/IGBF-1188-SynonymFile

Moving it to First level review column.
Requesting to review and test dependent functionalities too to avoid breaking of other modules.
Ivory Blakley assigning it to you.

I don't quite follow the logic here:

private static boolean updateSynonymFile(...) {
...
if (xsynonymFile.getText().isEmpty() || synonymFileStatus)

else if(!synonymFileStatus)

else

return true; ... [D]
}

How do you ever reach scenario C? if one or both parts are true -> scenario A. If neither is true -> B. Nothing left for C.
The return statement at the end [D] is basically only there for the sake of scenario A, so it might be clearer if the return statement was in the if statement. Make it clear what kind of case each clause is intended to handle.

git review:
one commit, good message. Good!

code review:
I like the tactic of changing void methods to return booleans.
I don't see any problems, but there are a couple places where I don't totally follow (see comment about logic flow).

functional review:
In "Please upload a valid synonyms file" the term upload is bit misleading. "select" is probably better. The file contents are not saved, just the files location. So if I edit my file after selecting it, the updates are reflected the next time I open a genome. If I delete it, or make it invalid, then the synonyms I previously had are lost.
Functionally this looks good. When I add a proper chromosome.txt file or synonyms file, all is well. But when I remove the synonyms from a given line I get a warning. I can dismiss the warning and move on. If the file is bad, and I don't choose to check it, there is no warning. I'm not sure if we would want to remove the file path if it is determined to be bad.
Just having the path displayed even if it is not working is a bit misleading. It would be nice if we could either

• remove the file path if we determine the file there is no good, or
• make some visible change in the display, maybe display the text in red, or put a red asterisk next to the file path window if the file is no good.

Overall, this looks good, but I am sending it back to Sneha to tweak the things I mentioned.

• make sure the cases in the updateSynonymFile method are clear
• change "upload" to "select" (might want to consult Ann on this to be sure)
• Add some change to the window appearance when the file is not valid (or remove the file path)... (might want to consult Ann on this)

Ivory Blakley : Did you compare the functionality with the working master branch? I did not do anything extra which is not done in master so I did not clear the path of the file incase it's invalid.

To answer the above query regarding reaching case C, It was present in master branch so i dint get rid of it. But now when I checked all combinations then we don't need the else clause so I will take it out. Thanks for pointing it out Ivory Blakley

I have made changes and pushed them to my fork.
However, clearing the path is something which is different from the scope of this issue and we shall discuss it with Professor and fix it in next sprint. What do you say? Ivory Blakley

I don't see any way we would have gotten to the else case, but I think it is still best practice to include an 'else' option, in case the system somehow throws you a curve ball. I didn't mean to say earlier that it was not needed, just that the logic was not clear.
I think you are right to say that all possibilities fall into the first two options.
If we only have two options, then we could simplify the code even further and change the else-if statement into an else statement.

I also think it would be best to move the return statement (currently line 191) which only applies in the if case into that code block (currently starts at line 184)

// either there is no synonyms file to use, or there is one and it was used successfully
if (xsynonymFile.getText().isEmpty() || synonymFileStatus)

• PreferenceUtils.getLocationsNode().put(PREF_FILE_URL_KEY, xsynonymFile.getText());
• return true;

// all other cases
else

• ErrorHandler.errorPanel("Unable to Load Personal Synonyms - " + PREF_FILE_URL_KEY,
• "Please select a valid synonym file.", Level.SEVERE);
• return false;

I recommend making this simplification (move the return statement, change "else if" to "else")
I included comments in how I drafted the code above, but I think the code changes are enough that the comments are no longer necessary.

@Sneha, its your issue, so its your call; these are my recommendations.

After making these changes, rebase onto master and submit a pull a request.

After considering the recommendations, I think that the case "else if " is the only else condition possible so i am keeping it the same.
Moving return statement will not affect anything because it will surely satisfy one of the cases and it returns true if everything works or false if there is a failure. (The same happens even if I move it in "if" block.)
I am rebasing and submitting PR.

When testing this story, I am running into two things that may still require attention:
1. I am receiving the pop-up error that says the file is invalid, but I am unable to close the window when clicking the "Ok" button the first time. It seems that I have to click the "Ok" button twice to close the error window.
2. Also, it seems the filepath text is getting populated to the synonyms textbox even if the pop-up error says the file is invalid. It seems like we may want to prevent the file from getting populated to the textbox if the file is invalid.

I am moving back to the To Do column and reassigning to Ann to determine next steps.

Answering Mason's comment:

"1. I am receiving the pop-up error that says the file is invalid, but I am unable to close the window when clicking the "Ok" button the first time. It seems that I have to click the "Ok" button twice to close the error window."

Please change code so that user needs only to click the "Ok" button once, not twice, to close the error window.

"2. Also, it seems the filepath text is getting populated to the synonyms textbox even if the pop-up error says the file is invalid. It seems like we may want to prevent the file from getting populated to the textbox if the file is invalid."

Can you attach an image showing this?

Not having seen it yet, this is my comment: The user needs to see the path to the invalid file in order to evaluate the error and determine next steps. For instance, the user might have accidentally selected the wrong file. If the text entry (textbox) field is the only place the invalid file path is shown, then the current behavior needs to continue. It is less than ideal if it requires more work from the user to then select the correct value.

For the first issue :
The reason system is asking us to click "OK" twice is because there are two pop-ups generated that are stacked upon each other.

Where is the file path getting filled in? (Mason's #2 comment)

The file path is getting populated in the Data Source tab in the Preferences window as shown in the attached screenshot

So as suggested in one of the comments above shall I change the color of the text (file path) to RED if the selected file is invalid?

Need code review for the issue,
Please find the code at the below link https://bitbucket.org/kkorey/kkorey-igb/branch/IGBF-1188-synerror

Kiran Korey
A suggestion. Can we make the color a little bit on the red side rather than the pink side?
Its not indicating the user that its an error! Also how about making only the text red instead of changing foreground and background?
Did not find any issues in Functionality.
Both the errors mentioned by Mason are resolved.

I have a concern. Because the text fields are editable, even after uploading the user can make changes to the path or completely remove it. So what if we make the fields not editable ?
Assigning back to Kiran Korey. Please drop a comment here when you have considered these suggestions.

Sneha Ramesh Watharkar
Professor asked me to use the same colors as what is being used for "Invalid Data Source".

And for the editable texts, If the user wants to remove the selected file the only option they have is to edit the text box, so we cannot make the fields not editable.

Agreed!

Submit PR

PR merged into master

Tested, works fine.