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] getHangingProtocolModule Name or Id ? #3755

Open
salimkanoun opened this issue Oct 31, 2023 · 6 comments
Open

[Bug] getHangingProtocolModule Name or Id ? #3755

salimkanoun opened this issue Oct 31, 2023 · 6 comments

Comments

@salimkanoun
Copy link
Contributor

Describe the Bug

It seems to be a discordance between documentation and code

in the documentation : https://docs.ohif.org/platform/services/data/hangingprotocolservice/#protocol-definition
Hanging protocol should be registered by Id

But in the code they are registered by name property :

function getHangingProtocolModule() {

Also the url param to set a HP is hangingProtocolId so I guess there is a error in the code (should use id instead of name, and will led to breaking change)

Steps to Reproduce

None

The current behavior

registering of HP use name

The expected behavior

should use id property

OS

linux

Node version

20

Browser

Chrome 100+

@salimkanoun salimkanoun added the Awaiting Reproduction Can we reproduce the reported bug? label Oct 31, 2023
@sedghi
Copy link
Member

sedghi commented May 4, 2024

I think id makes more sense, you are saying URL is based on name?

@salimkanoun
Copy link
Contributor Author

The weird thing is that getHangingProtocolModule is mapping the id of the hanging protocol to a "name" property

The HP object has itself a "name" and an "id" property, so I guess getHangingProtocolModule could simply return array of protocols. If needed to add a differentiating key then I guess it would be better to expose "id" instead of "name" in this getHangingProtocolModule

@linear linear bot changed the title [Bug] getHangingProtocolModule Name or Id ? [Bug] getHangingProtocolModule Name or Id ? Nov 6, 2024
Copy link
Member

sedghi commented Jan 15, 2025

I hate this, maybe next next version we can fix it

@sedghi sedghi removed the Awaiting Reproduction Can we reproduce the reported bug? label Jan 15, 2025
@sedghi sedghi added the Bugs Bug reported, reproducible, and verified. label Jan 15, 2025 — with Linear
@sedghi
Copy link
Member

sedghi commented Feb 3, 2025

I took a closer look at this issue. The problem lies in the following code:

{
      name: defaultProtocol.id,
      protocol: defaultProtocol,
    },

This is causing the protocol module registration to hang. Our modules, such as the viewport module and panelModule, are identified by their name. However, what really makes a Hanging Protocol (HP) unique is its id. So, when you use a query parameter like ?hangingProtocolId=[HP id], it should be referencing the HP by its id.

I'm making a PR to just make the id case insensitive, maybe that helps a bit

@sedghi sedghi removed the Bugs Bug reported, reproducible, and verified. label Feb 3, 2025
@salimkanoun
Copy link
Contributor Author

humm didn't understand so much but will look at the PR ^^

@sedghi
Copy link
Member

sedghi commented Feb 3, 2025

What I mean is that all modules register with a unique name. For example, you can see this in the documentation for data source modules at https://docs.ohif.org/platform/extensions/modules/data-source/ and panel modules at https://docs.ohif.org/platform/extensions/modules/panel/. Here, we have both an id and a name for HP, which is pretty similar to having a name for internal use and a readableName for UI purposes. It's really not a big deal, we can just treat the id as the actual name and the name as the readable name.

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

2 participants