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

Add: (get/set) (color/class/mode) #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

mnasan
Copy link

@mnasan mnasan commented Mar 8, 2022

No description provided.

@magednasan
Copy link

New functions are added to this plugin:
viewer.annotations.get_color(): return the current stroke's color for annotations.
viewer.annotations.set_color(c): set a color for the next annotation, c can be a string like 'red', 'blue', .. or '#FF0000' format.
viewer.annotations.get_mode(): return the current mode of the viewer, it can be either {'DRAW' or 'MOVE'}.
viewer.annotations.set_mode(): set the mode of the viewer to {'DRAW' or 'MOVE'}, this function takes only those two string values as input.
viewer.annotations.set_class(): set a specific HTML class for each annotation (in the <path> tag). New style can be added to specific annotation based on class name. (We are assuming to draw several annotations in different colors and different classes).
viewer.annotations.get_class(): return the current active class the is used for drawing.

@iangilman
Copy link

I don't have the power to merge things here, and I'm not sure if @emigre is still maintaining this project, but I thought I'd add my two cents:

For starters, I think it's awesome that you've added all these new features! Definitely seems more powerful.

Secondly, though, looking at the changes, it looks like there are quite a number of changes that are unrelated to these new features... Looks like a complete overhaul of the plugin! Typically, with a pull request it's good to focus on just one feature at a time, and at the very least to explain what all the changes are for.

Thirdly, I think function names like getColor would be more in-line with the OSD naming convention, if you wanted to match that in this plugin.

Anyway, of course it's up to @emigre how they want to proceed. Even if they aren't still maintaining the project, it's good to have this PR here so that it can help other people who want these features!

If it does turn out @emigre has abandoned this repo, you might consider making your version an official fork. We can update http://openseadragon.github.io/#plugins to point to it.

@emigre
Copy link
Owner

emigre commented Mar 10, 2022

hey guys hi. I've been absent for a while, but to be honest this plugin has been the last of the priorities in my life for a while - I was thiking on getting rid of it somehow, to be honest. @iangilman thanks for keeping an eye, you are really great, I'm amazed that you've maintained the attention on the plugin for so long. Very nice of you.

@magednasan thanks for the contribution, but, as @iangilman was saying, there is a lot of stuff here, some of them are changes that are not heading in the direction I had in mind. As an example, I did not have in mind to support bower. Also the change seems unrelated to the rest. All I'm sayin is, you are adding several things in one go.

I really don't know what would be the best way to handle this. In my mind I was going to remove preact from this plugin and implement something simpler to update the SVG. I had an idea on how to do this, which would end up with the plugin not depending on any library. I'll have ot think what to do.

@emigre
Copy link
Owner

emigre commented Mar 10, 2022

@iangilman as the "guardian"of the OpenSeadragon ecosystem, perhaps you have ideas on what would be the best? Feel free to email me or contact me in any way if you want to discuss, be involved or advice. Cheers.

@emigre
Copy link
Owner

emigre commented Mar 10, 2022

To give you both some context around this, I created this as a mindless experiment years ago with no particular intention. I didn't add unit tests, I did not use TypeScript, I used an already existing rendering library... In short I didn't think about it too much. But then over time I've received a number of PRs to it. It seems that some people are actually interested in the plugin. To my surprise!....

@iangilman
Copy link

@emigre Good to hear from you! Yes, there certainly does seem to be interest in this plugin! I think the big annotation option for OSD is https://github.com/recogito/annotorious-openseadragon but I think this plugin has a lot of appeal because it's simpler and doesn't depend on other things, so I think there's value in both.

I would certainly be happy if you wanted to continue to support and evolve this plugin. If @magednasan's changes aren't to your liking, it's your perogative to reject this PR or ask for changes to it. You should definitely take it in whatever direction you feel is best! Of course if you don't want to continue to maintain it, maybe making an announcement in the issues or the readme saying you're looking for a new maintainer would be good.

I'm curious to hear what @magednasan thinks... If they want to pick up the torch, or if they're happy to leave it as it is.

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.

None yet

4 participants