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

feat: create idcbrowser:// protocol to interact with slicer #43

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

vkt1414
Copy link
Collaborator

@vkt1414 vkt1414 commented Jul 26, 2024

This was a project born during PW 40 when @pieper once mentioned the idea of using Slicer just the way we use zoom. This post showed that it was indeed possible, and the current implementation is inspired from it, and the slicerio package.

See here for more discussion:

https://projectweek.na-mic.org/PW40_2024_GranCanaria/Projects/Launch3DslicerViaClickableUrlsForViewingIdcDataViaSliceridcbrowserAndIdcIndex/

https://discord.com/channels/843934857620357130/1197786173637132368

While the progress made during that PW was good, there was room for improvement as the previous approach was launching a new slicer instance with every URL request.

This PR aims to address some of the deficiencies. It is now possible to reuse the slicer instance. The key component to achieving this was using a web server. Significant progress was made during the next project week (PW41 at MIT) even though there was no official continuation of the project on the PW41 page. Once the webserver was able to handle localhost requests, the idcbrowser:// custom protocol was designed to launch a Python script (using PythonSlicer binary) that will parse the URL and send the requests to the slicer webserver.

It's been tested on all three major OSs Ubuntu, Windows, and MacOS. On Ubuntu, only the Firefox browser works. On Windows and MacOS any browser can handle idcbrowser:// requests. However, localhost:2042/idc requests work seamlessly on any browser/OS and are also clickable. idcbrowser:// is not recognized as a URL in markdown due to security reasons. However, in MS Excel, hyperlinks were recognized as URLs.

Pending tasks are:

  • Designing the expected behavior when a user has multiple slicer apps installed
  • Load segmentation files properly

It took a village (huge thanks to @pieper, @lassoan, @jcfr, @fedorov, @deepakri201, and @ccosmin97) to get to this phase but this was a very cool project, and hope others can take it to further heights in the future.

@vkt1414 vkt1414 requested review from pieper and fedorov July 27, 2024 00:40
@vkt1414 vkt1414 marked this pull request as ready for review July 27, 2024 00:40
vkt1414 added 2 commits July 28, 2024 03:19
use shell quote to be resilient to spaces in paths
add acknowledgement text
reformat code using black code formatter
@fedorov fedorov changed the title feat: create idcrowser:// protocol to interact with slicer feat: create idcbrowser:// protocol to interact with slicer Jul 29, 2024
Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

Thanks for getting this working @vkt1414 👍

I haven't had a chance to try it, but I'm back from vacation now so we should be able to get a good version of this going. What I'd like to do is have you give a demo, possibly during the IDC meeting today or at some other convenient time and then walk through the python code.

The one thing I'm concerned about is the concept of the 'resolver script' -- at first review that seems to add extra complexity to me, but perhaps you can explain.

@vkt1414
Copy link
Collaborator Author

vkt1414 commented Jul 30, 2024

Sure @pieper I may or may not be able to join today's weekly as I have a dentist appointment. If I can join, I will show the demo today itself.

As for the resolver script, it simply parses the idcbrowser:// URL entered by the user, and then passes along the request to the slicer webserver. We can definitely discuss more when we can meet.

@fedorov
Copy link
Member

fedorov commented Aug 13, 2024

@pieper @vkt1414 would it make sense, to be safe, to initially have the installation of this feature to be disabled by default, so that it can be toggled/enabled by the user? I am a bit worried of using the production extension in the nightly for testing.

Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

This looks cool - I'm looking forward to giving it a try 👍

Just a few questions/suggestions and then I think we should merge it for testing.

@pieper
Copy link
Member

pieper commented Aug 13, 2024

feature to be disabled by default

Yes, in the ideal world this should also include an uninstaller that resets everything. Then you could have a UI option to install and uninstall the feature, with the default being off in the testing phase.

@pieper
Copy link
Member

pieper commented Aug 13, 2024

Another suggestion: in the resolver.py script there should be some error checking and also to try launching Slicer if the server is not responding. We should think about how best to communicate to the user about what is going on (i.e. if there are any error message or other feedback to display).

@vkt1414
Copy link
Collaborator Author

vkt1414 commented Aug 13, 2024

Thanks @pieper for the review. I'll make suggested changes and test them.

feature to be disabled by default

Yes, in the ideal world this should also include an uninstaller that resets everything. Then you could have a UI option to install and uninstall the feature, with the default being off in the testing phase.

I wish I knew qt to implement this..as I believe all we need is fetch and execute a script upon clicking a button in slicer extension UI.

@vkt1414
Copy link
Collaborator Author

vkt1414 commented Aug 13, 2024

Another suggestion: in the resolver.py script there should be some error checking and also to try launching Slicer if the server is not responding. We should think about how best to communicate to the user about what is going on (i.e. if there are any error message or other feedback to display).

We can certainly invoke slicer if it is not running yet. I'll try to add that. Currently I do display a message when a series is downloaded and loaded into slicer. We can show something like if there is no instance of slicer web server running.

responseBody = f"Error downloading or indexing UID(s): {e}".encode()

@vkt1414
Copy link
Collaborator Author

vkt1414 commented Aug 13, 2024

@pieper @vkt1414 would it make sense, to be safe, to initially have the installation of this feature to be disabled by default, so that it can be toggled/enabled by the user? I am a bit worried of using the production extension in the nightly for testing.

May I know the safety concerns? This feature does impact a regular user who just wants to browse

@fedorov
Copy link
Member

fedorov commented Aug 13, 2024

May I know the safety concerns? This feature does impact a regular user who just wants to browse

IMHO, anything that modifies Registry on Windows has at least a potential for causing problems. To ease the rollout and reduce the risks, to me it would make sense to make this feature available for testing for those who want to enable it knowingly. We can enable it by default in a subsequent update. It does not need to be done by GUI - this could be a setting flag initialized in the Slicer config.

@vkt1414
Copy link
Collaborator Author

vkt1414 commented Aug 13, 2024

modifies Registry on Windows has at least a potential for causing problems.

You'd surprised that even though editing registry is locked on MGB official PCs with GUI, there were no issues modifying with code! I do not disagree with other points. And I have no idea about slicer config

@pieper
Copy link
Member

pieper commented Aug 13, 2024

install and uninstall the feature, with the default being off in the testing phase.

I wish I knew qt to implement this

My thought would just to be the opposite of registering the custom protocol - just delete all the files and registry keys you create in the registration step.

@lassoan
Copy link

lassoan commented Aug 13, 2024

This is really nice! The only think I would change is to get user consent before permanently changing any computer configuration. Instead of silently calling self.registerCustomProtocol() on every startup, a popup could be displayed where the user could approve the configuration change, reject it, or choose to decide later.

@vkt1414
Copy link
Collaborator Author

vkt1414 commented Aug 13, 2024

This is really nice! The only think I would change is to get user consent before permanently changing any computer configuration. Instead of silently calling self.registerCustomProtocol() on every startup, a popup could be displayed where the user could approve the configuration change, reject it, or choose to decide later.

Thanks @lassoan ! I really like this suggestion.

@fedorov
Copy link
Member

fedorov commented Aug 27, 2024

@vkt1414 are you still working on this, do you plan to address the suggestion in #43 (comment) and suggestions from Steve?

@vkt1414
Copy link
Collaborator Author

vkt1414 commented Aug 27, 2024

@vkt1414 are you still working on this, do you plan to address the suggestion in #43 (comment) and suggestions from Steve?

I would not be able to work on the Qt component anytime soon. It would be great if someone with qt expertise can take over that part. @LennyN95 suggestion to solve this was to create a file to record the state of the user's preference for two things, like a database.

  1. Interested in the custom protocol or not
  2. Whether to stop showing the pop up the message again

Both of these will require qt components that I'm not familiar and will require significant time to able to implement

@pieper
Copy link
Member

pieper commented Aug 27, 2024

Thanks for the update @vkt1414. We'll try to get someone with Slicer Qt development experience to work on this.

@LennyN95
Copy link

It'd be also great if the user could specify the port the server is running on. Although conflicts are unlikely, giving the user a) feedback which port is occupied and b) the option to manually update this will provide transparency and allow to resolve any potential conflicts.

@fedorov
Copy link
Member

fedorov commented Aug 28, 2024

@vkt1414 sounds good, thank you for the update!

@pieper I think for the Qt part, this could be as simple as adding two buttons to a section of the existing module interface - to register and to un-register the custom protocol.

@fedorov
Copy link
Member

fedorov commented Aug 28, 2024

We tested this branch with @ccosmin97 and @deepakri201 on mac. It worked for Deepa. When trying to open this URL idcbrowser://series/1.2.840.113654.2.55.154809705591242159075253605419469935510 I was prompted to open the app, but nothing happened in Slicer. I see the messages below in the python console, but nothing happens. Another note is that Slicer app does not become modal.

SlicerStartupCompleted emitted
IDC Request Handler has been registered and server started.
Slicer URL protocol is already registered.
image

Testing on Linux JetStream, I am getting this runtime error on startup, which makes sense, since idc-index won't be installed - the main module asks for the user confirmation to install it on the first start. So this needs to be fixed.

image

After installing idc-index it worked in linux!

@fedorov
Copy link
Member

fedorov commented Aug 28, 2024

@LennyN95 had some good ideas/questions as follow up to this, so maybe we should meet to discuss @pieper, since I do not have answers for Leo!

@pieper
Copy link
Member

pieper commented Aug 28, 2024

These all sounds fixable, just a matter of investing some time in it. I'm happy to meet and discuss.

We can use Slicer's Qt environment to give feedback to the user if something fails.

The one piece of the process that needs to be implemented that's not a slicer-specific coding task is the functions to undo all the installation steps on each platform (i.e. removing any files and registry keys. If someone else takes on this task I could do the Slicer/Qt parts.

@vkt1414
Copy link
Collaborator Author

vkt1414 commented Aug 28, 2024

undo all the installation steps on each platform (i.e. removing any files and registry keys. If someone else takes on this task

I can help with undoing the changes, but will not be able to test until the weekend after the upcoming holiday weekend.

But if someone would like to go ahead..we will need a function like this to remove the folder/files

import os
import platform
import winreg as reg
import logging

# Configure logging
logging.basicConfig(level=logging.WARNING)
logger = logging.getLogger(__name__)

def remove_protocols():
    system = platform.system()

    if system == "Windows":
        try:
            key = reg.OpenKey(reg.HKEY_CURRENT_USER, r"Software\Classes", 0, reg.KEY_ALL_ACCESS)
            reg.DeleteKey(key, "idcbrowser")
            reg.CloseKey(key)
            logger.info("Registry key 'idcbrowser' removed successfully.")
        except FileNotFoundError:
            logger.warning("Registry key 'idcbrowser' not found.")

    elif system == "Linux":
        path = os.path.expanduser("~/.local/share/applications/idcbrowser.desktop")
        try:
            os.remove(path)
            logger.info(f"File '{path}' removed successfully.")
        except FileNotFoundError:
            logger.warning(f"File '{path}' not found.")

    elif system == "Darwin":  # macOS
        path = os.path.expanduser("~/Applications/slicer-app.app/")
        try:
            os.rmdir(path)
            logger.info(f"Directory '{path}' removed successfully.")
        except FileNotFoundError:
            logger.warning(f"Directory '{path}' not found.")
    else:
        logger.warning("Unsupported operating system.")


@fedorov
Copy link
Member

fedorov commented Aug 29, 2024

These all sounds fixable, just a matter of investing some time in it. I'm happy to meet and discuss.

@pieper I agree most should be easy to address, with the exception of the one where it just did not work for me on my mac as I described in the issue. I am not sure what is going on there.

I will also test on Windows a bit later.

@vkt1414
Copy link
Collaborator Author

vkt1414 commented Aug 29, 2024

These all sounds fixable, just a matter of investing some time in it. I'm happy to meet and discuss.

@pieper I agree most should be easy to address, with the exception of the one where it just did not work for me on my mac as I described in the issue. I am not sure what is going on there.

I will also test on Windows a bit later.

From what you mentioned, slicer-app.app is already present on your system. Could you remove it and try again? as I currently do not overwrite if one is already present

also is localhost:2042/idc/collections working? if so, slicer webserver is working as expected, it is the cutom protocol that is not working properly, which pin points to slicer-app.app

@pieper
Copy link
Member

pieper commented Aug 29, 2024

For configuring persistent properties, like whether the url handler is enabled or not, what port to use, and perhaps which slicer to run if there are multiples versions installed, we could use the QSettings and add a scripted preferences panel to the Application settings dialog (see for example how it is done in the DICOM module).

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.

5 participants