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

Bug About Loading Solid Color Scheme Maps on another Map #726

Closed
umut-er opened this issue Jul 4, 2024 · 9 comments
Closed

Bug About Loading Solid Color Scheme Maps on another Map #726

umut-er opened this issue Jul 4, 2024 · 9 comments
Assignees
Milestone

Comments

@umut-er
Copy link
Contributor

umut-er commented Jul 4, 2024

Here is a bug I detected with item 4 (file operations) of issue #725. This is specifically about color schemes.

If a sample or file with color scheme of type solid is loaded on top of a another already existing map of color scheme gradient or 3D, the colors won't be rendered correctly on the map even though it will be shown correctly on the sidebar. Refer to the video below:

Screen.Recording.2024-07-04.at.12.24.13.mp4

I determined that this issue is caused by not properly deleting the background image. I tried to fix this and it seems to be gone. Here is the code I changed (this is in newt app-menu.js lines 823-839):

if(currentGeneralProperties.mapColorSchemeStyle == 'solid'){
  for(var nodeClass in appUtilities.mapColorSchemes[currentGeneralProperties.mapColorScheme]['values']){
    classBgColor = appUtilities.mapColorSchemes[currentGeneralProperties.mapColorScheme]['values'][nodeClass];
    // nodeClass may not be defined in the defaultProperties (for edges, for example)

    // NEW CODE
    var bgObj = chiseInstance.elementUtilities.getBackgroundImageObjs(cy.nodes());
    chiseInstance.removeBackgroundImage(cy.nodes(), bgObj);

    // OLD CODE
    // if(nodeClass in chiseInstance.elementUtilities.getDefaultProperties()){
    //   chiseInstance.undoRedoActionFunctions.setDefaultProperty({class: nodeClass, name: 'background-image', value: ''});
    //   chiseInstance.undoRedoActionFunctions.setDefaultProperty({class: nodeClass, name: 'background-color', value: classBgColor});
    // }
  }
}

I found the new code after inspecting how the manual removal of background images works. You can see the button related to that:

image

and here is the code related to that button (found at newt inspector-utilities.js lines 817-821):

$('#inspector-delete-bg').on('click', function () {
  var bgObj = chiseInstance.elementUtilities.getBackgroundImageObjs(selectedEles);
  chiseInstance.removeBackgroundImage(selectedEles, bgObj);
  updateBackgroundDeleteInfo();
});

as you can see, I copy pasted it almost one-to-one and it seems to be working. However, I don't exactly understand what the old code does and why it doesn't work. I think someone more knowledgable than me should review this change and insure that it doesn't get rid of other functionalities that we might want.

@umut-er umut-er self-assigned this Jul 4, 2024
@umut-er
Copy link
Contributor Author

umut-er commented Jul 4, 2024

My commit about this issue is misattributed to #725. Here is the commit: a3a52a1

@umut-er
Copy link
Contributor Author

umut-er commented Jul 4, 2024

Btw, the way to test this easily is this:

  1. Load Neuronal Muscle Signaling (this has 3D color scheme)
  2. Load Insuling like growth factor (IGF) signaling (this has solid color scheme).

The changes are now live on the internal server.

@hasanbalci
Copy link
Contributor

@umut-er The code you commented is required to set default properties of the map based on the properties read from the file. For example, open Newt, set color scheme to Gradient, open Insulin sample (with solid color scheme) and try to add new nodes to map. You will see that new elements added don't use the color scheme of Insulın but the Gradient.

@umut-er
Copy link
Contributor Author

umut-er commented Jul 4, 2024

@hasanbalci What if I uncomment the old code as well? Do you think that is a good idea?

@hasanbalci
Copy link
Contributor

@umut-er I think it should work. So please uncomment the previous code and keep the new one.

@umut-er
Copy link
Contributor Author

umut-er commented Jul 4, 2024

There is another issue as well. If you try to load a plain sbgnml map (not a sample this time) on top of a gradient or 3D the colors are still off. Here is the cause of this issue: After loading a new map, we look for map properties defined inside of it. Since plain sbgnml files don't have such information (compare samples of type .nwt, which have that information), we do not correctly reset the color scheme. I will make a fix for this as well and push them together.

@umut-er
Copy link
Contributor Author

umut-er commented Jul 4, 2024

Or is this intended? There are some comments that seem to indicate that this is intended. I am not going to make any changes about the comment above.

umut-er added a commit that referenced this issue Jul 4, 2024
@umut-er
Copy link
Contributor Author

umut-er commented Jul 4, 2024

@hasanbalci I did as you requested. The changes are now live on the internal server. Can you review please?

@umut-er umut-er assigned hasanbalci and unassigned umut-er Jul 4, 2024
@hasanbalci
Copy link
Contributor

It seems working well, thanks. For the other issue, I think it is ok to use the current color scheme when we load SBGN plain files.

@ugurdogrusoz ugurdogrusoz added this to the version 4.0 milestone Jul 4, 2024
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

No branches or pull requests

3 participants