Panoramax: Adopt "copy id" feature from mapillary #10856

MohamedAli00949 · 2025-03-10T07:40:17Z

Hi there,

This is my code to add the copy id button at the Panoramax image

Changes

"Copy ID" button component.

Issue

tordans · 2025-03-10T09:12:16Z

From reading the code I don't see how this PR could resolve the requirements described in the issue, specifically #10611 (comment) and #10611 (comment)

MohamedAli00949 · 2025-03-10T09:24:06Z

As I understood the issue, I found the requirements are to add the copy id button to the photo viewer, such as below, and research imagery services such as Panoramax photo viewer. I have replied with the research results.

So, what is wrong with my implementation? and How to improve it.

Thanks in advanced

MohamedAli00949 · 2025-03-10T15:29:01Z

@tordans, Do you mean that I haven't used the panoramax=* field and want to use it at the copy button instead of passing it to the function or another thing?

MohamedAli00949 · 2025-03-10T21:43:02Z

@tordans, after reading the related modules to this feature, I have found another solution to solving this problem using hash (search hash params class, or using context). Is this suitable?

tyrasd

it should work like the corresponding button on mapillary photos:

MohamedAli00949 · 2025-03-11T16:19:52Z

Ok, But I will use the copy icon instead of the plus

MohamedAli00949 · 2025-03-12T05:29:17Z

Hi @tyrasd,
I have changed the component, but the street-side photo viewer appears as the following.

I suggest the following style:

If my suggestion is good, I will commit these changes.

tyrasd · 2025-03-12T11:29:57Z

please use the same icon as used in the mapillary viewer. otherwise this would be very confusing.

tordans · 2025-03-12T12:31:58Z

@tyrasd I think the main thing we need to agree on first is, what solution does resolve the linked issue. I outlined my proposal in the links above. The core idea of that is, to use what we have for Mapillary and abstract it to reuse of other image providers. This is not what is being proposed here as far as I understand the code but instead a copy button that would require to manually add the key and paste the value. As a result we would have two features doing something very similar – one better then the other.

My recommendation would be to clarify what we are looking for so this PR might be adjusted … or otherwise closed as it is not what we should merge IMO.

tyrasd · 2025-03-12T12:52:21Z

The core idea of that is, to use what we have for Mapillary and abstract it to reuse of other image providers.

I agree, that's basically what I meant with it should work like the corresponding button on mapillary photos above. 🤷

A copy-to-clipboard button is definitely not going to cut it here.

gedankenstuecke · 2025-03-12T13:01:38Z

I've rephrased the original issue I had opened to make it clearer that the "agreed" solution is copying the mapillary-functionality

MohamedAli00949 · 2025-03-13T02:59:16Z

I am so sorry, for the misunderstanding.
Latterly, I added the functionality to set the photo button which was implemented before at mapillary.
But, there's to other issues,

The first is the plus icon, which provides a bad experience and I suggest replacing it with the following icon (such as @tordans has been suggested before at mapillary implementation).

The second is the position of the zoom icons in the panormax photo as in the following image and I suggest moving them down a bit.

If one of my suggestions or both isn't right tell me to change them.

tordans

Thanks for the update!

tordans · 2025-03-13T04:59:45Z

modules/ui/photoviewer.js

-                    .every(value => value === activeImage?.id)) {
+                const entitiesWithTag = entities.map(entity => entity.tags[tagName] ? entity.tags[tagName] : undefined).filter(e => e);
+                const isPhotoUsed = entitiesWithTag
+                    .some(value => value === activeImage?.id);


This part checks if the "looked at" image id is already present in the tags value, right?
We should make this something like value.includes(activeImage?.id) to account for the edge case that multiple image ids are used separated by ;, eg. "<id1>;<id2>". (We talked about that practice while reviewing openstreetmap/id-tagging-schema#1344)

Interesting to see the change from every to some. That was a bug in the previous implementation, right?

Interesting to see the change from every to some. That was a bug in the previous implementation, right?

This is right, I have found this bug and I have rewritten this line to refactor the function.

This part checks if the "looked at" image id is already present in the tags value, right? We should make this something like value.includes(activeImage?.id) to account for the edge case that multiple image ids are used separated by ;, eg. "<id1>;<id2>". (We talked about that practice while reviewing openstreetmap/id-tagging-schema#1344)

Okay, this is more effective, I will apply it.

Thanks, a lot.

tordans · 2025-03-13T05:03:30Z

modules/ui/photoviewer.js

-            if (services.mapillary.isViewerOpen()) {
-                if (context.mode().id !== 'select' || !(layerStatus('mapillary') && getServiceId() === 'mapillary')) {
+            const service = getServiceId();
+            const isActiveService = ['mapillary', 'panoramax'].includes(service) && services[service].isViewerOpen();


Lets extract this array into something like a const PHOTO_SERVICES_WITH_TAG_BUTTON = [] to give it a bit more visibility that this is the place where we configure the services. Please check the code conventions on such const (naming in placement in file); I don't know how that is done elsewhere in the codebase.

We are also making the assumption here that the service name and tag key will be the same. That is true for those two but might be different for others. We could account for this now… but it feels like premature optimization so I would do it in a refactoring later when the need comes up.

tordans · 2025-03-13T05:06:33Z

modules/ui/photoviewer.js

-                if (context.mode().id !== 'select' || !(layerStatus('mapillary') && getServiceId() === 'mapillary')) {
+            const service = getServiceId();
+            const isActiveService = ['mapillary', 'panoramax'].includes(service) && services[service].isViewerOpen();
+            const mapillaryOrPanoramax = (layerStatus(service) && ['mapillary', 'panoramax'].includes(service));


Lets use a more generic name for the const

Lets reuse the same array defined once above

Can you double check the conditions, I think that isActiveService already accounts for the second condition in this check, right?

tordans · 2025-03-13T05:06:49Z

modules/ui/photoviewer.js

+            const mapillaryOrPanoramax = (layerStatus(service) && ['mapillary', 'panoramax'].includes(service));
+
+            if (isActiveService) {
+                if (context.mode().id !== 'select' || !(mapillaryOrPanoramax)) {


Suggested change

if (context.mode().id !== 'select' || !(mapillaryOrPanoramax)) {

if (context.mode().id !== 'select' || !mapillaryOrPanoramax) {

tordans · 2025-03-13T05:09:25Z

modules/ui/photoviewer.js

-                function setMapillaryPhotoId() {
-                    const service = services.mapillary;
-                    const image = service.getActiveImage();
+                function setMapillaryOrPanoramaxPhotoId() {


Like above, lets look for a name that is more generic.

What would be a good name to describe the general feature? "addPhotoServiceTag", "applyTagOfPhotoService",…

Yeah, I think addPhotoServiceTag is more expressional to its implementation.

tordans · 2025-03-13T05:12:18Z

modules/ui/photoviewer.js


                const graph = context.graph();
                const entities = context.selectedIDs()
                    .map(id => graph.entity(id));

-                if (entities.map(entity => entity.tags.mapillary)
-                    .every(value => value === activeImage?.id)) {
+                const entitiesWithTag = entities.map(entity => entity.tags[tagName] ? entity.tags[tagName] : undefined).filter(e => e);


That should be the same but shorter, right?

Suggested change

const entitiesWithTag = entities.map(entity => entity.tags[tagName] ? entity.tags[tagName] : undefined).filter(e => e);

const entitiesWithTag = entities.map(entity => entity.tags[tagName]).filter(Boolean);

nice, thanks

tordans · 2025-03-13T05:17:58Z

Given the research you did in #10611 (comment) I only see on other service that has a dedicated wiki page about a dedicated tag which is https://wiki.openstreetmap.org/wiki/Key:kartaview

I don't think Kartaview is especially active / used in OSM and the tag has low usage with ~600 https://taginfo.openstreetmap.org/keys/kartaview but we could look into adding this as well. The Kartaview UI might require extra work though, in which case I would rather skip it for later due to the low usage.

tordans · 2025-03-13T05:25:04Z

Re #10856 (comment)

Choosing the icon

Personally, I agree that the plus is a no go given the +/- for Zooming and that from the list we #10046 (comment) researched last time the one you chose is the best choice. Martin might see it differently but I suggest to go with that for now.

Position of button

Ideally we find one that works for all selected services
Ideally it is on the right because the tags are left of the preview (I think it feels unnatural to have the button on the right of the image viewer… but that might just be me)
Can we move the +/- via custom CSS for panoramax?
Otherwise we have to move the button. It looks like the best option is the bottom left picking a place that works for both services (given the different footer heights of both services); Ideally we don't need to change the position per service… — If so, that should might be something to abstract into the config CONST i mentioned in https://github.com/openstreetmap/iD/pull/10856/files/3e705dd7f72a52810a73bbeadca32b3fc158bf12#diff-f6d31cddb0ba27a935e3cb9e3aa4d4ec8ee2b68f92e4dcc26ca42c48724d7e9c

MohamedAli00949 · 2025-03-13T06:16:56Z

Given the research you did in #10611 (comment) I only see on other service that has a dedicated wiki page about a dedicated tag which is https://wiki.openstreetmap.org/wiki/Key:kartaview

I don't think Kartaview is especially active / used in OSM and the tag has low usage with ~600 https://taginfo.openstreetmap.org/keys/kartaview but we could look into adding this as well. The Kartaview UI might require extra work though, in which case I would rather skip it for later due to the low usage.

Yeah, this is right.
But I can't see this field used in schema fields.

MohamedAli00949 mentioned this pull request Mar 10, 2025

Panoramax: Adopt "copy id" feature from mapillary #10611

Open

tyrasd added the streetlevel An issue with streetlevel photos label Mar 10, 2025

tyrasd requested changes Mar 11, 2025

View reviewed changes

MohamedAli00949 force-pushed the enh/add-panoramax-copy-id branch from ddfda67 to 366dc0a Compare March 12, 2025 05:22

MohamedAli00949 added 2 commits March 13, 2025 04:33

feat: validate viewer status at panoramax

658d949

fix: adopt copy id feature with panoramax such as mapillary

3e705dd

MohamedAli00949 force-pushed the enh/add-panoramax-copy-id branch from 366dc0a to 3e705dd Compare March 13, 2025 02:49

tordans suggested changes Mar 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Panoramax: Adopt "copy id" feature from mapillary #10856

Panoramax: Adopt "copy id" feature from mapillary #10856

MohamedAli00949 commented Mar 10, 2025

tordans commented Mar 10, 2025

MohamedAli00949 commented Mar 10, 2025 •

edited

Loading

MohamedAli00949 commented Mar 10, 2025

MohamedAli00949 commented Mar 10, 2025

tyrasd left a comment

MohamedAli00949 commented Mar 11, 2025

MohamedAli00949 commented Mar 12, 2025

tyrasd commented Mar 12, 2025

tordans commented Mar 12, 2025 •

edited

Loading

tyrasd commented Mar 12, 2025

gedankenstuecke commented Mar 12, 2025

MohamedAli00949 commented Mar 13, 2025

tordans left a comment

tordans Mar 13, 2025

tordans Mar 13, 2025

MohamedAli00949 Mar 13, 2025

MohamedAli00949 Mar 13, 2025

tordans Mar 13, 2025

tordans Mar 13, 2025

tordans Mar 13, 2025

tordans Mar 13, 2025

MohamedAli00949 Mar 13, 2025

tordans Mar 13, 2025

MohamedAli00949 Mar 13, 2025

tordans commented Mar 13, 2025 •

edited

Loading

tordans commented Mar 13, 2025

MohamedAli00949 commented Mar 13, 2025

	if (context.mode().id !== 'select' \|\| !(mapillaryOrPanoramax)) {
	if (context.mode().id !== 'select' \|\| !mapillaryOrPanoramax) {

	const entitiesWithTag = entities.map(entity => entity.tags[tagName] ? entity.tags[tagName] : undefined).filter(e => e);
	const entitiesWithTag = entities.map(entity => entity.tags[tagName]).filter(Boolean);

Panoramax: Adopt "copy id" feature from mapillary #10856

Are you sure you want to change the base?

Panoramax: Adopt "copy id" feature from mapillary #10856

Conversation

MohamedAli00949 commented Mar 10, 2025

Changes

Issue

tordans commented Mar 10, 2025

MohamedAli00949 commented Mar 10, 2025 • edited Loading

MohamedAli00949 commented Mar 10, 2025

MohamedAli00949 commented Mar 10, 2025

tyrasd left a comment

Choose a reason for hiding this comment

MohamedAli00949 commented Mar 11, 2025

MohamedAli00949 commented Mar 12, 2025

tyrasd commented Mar 12, 2025

tordans commented Mar 12, 2025 • edited Loading

tyrasd commented Mar 12, 2025

gedankenstuecke commented Mar 12, 2025

MohamedAli00949 commented Mar 13, 2025

tordans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tordans commented Mar 13, 2025 • edited Loading

tordans commented Mar 13, 2025

MohamedAli00949 commented Mar 13, 2025

MohamedAli00949 commented Mar 10, 2025 •

edited

Loading

tordans commented Mar 12, 2025 •

edited

Loading

tordans commented Mar 13, 2025 •

edited

Loading