Thanks for filing this!

This should be an easy first task for somebody wanting to contribute to GNOME!
You should look around in mainWindow.js, https://git.gnome.org/browse/gnome-maps/tree/src/mainWindow.js#n142. And there you could update the shortcut. Maybe read-up on https://developer.gnome.org/gio/stable/GSimpleAction.html.

Allan: Do you feel that the Ctrl versions should be dropped or that both shortcuts should work?

Hi Jonas, Should I start working upon this one?

(In reply to amisha from comment #2)
> Hi Jonas, Should I start working upon this one?

It's open source, anyone can work on anything. :)

(In reply to amisha from comment #2)
> Hi Jonas, Should I start working upon this one?

That would be awesome! Do you need more information than given in comment 1 above?

Allan: Do you think the ctrl++ and ctrl+- should remain along side the - and + or should they be replaced?

Created attachment 314184 [details] [review]
Using plus and minus only for zoom in and zoom out purpose

Review of attachment 314184 [details] [review]:

Thanks!

I think the commit message can be improved. No need to have Gnome-maps in there. That is clear from where you are. Instead I think it should be something like 'MainWindow: Add simpler zoom accels'.
And no need to talk about a task. Maybe just say it is simpler to have just plus and minus.

You can look here for more guidence:
https://wiki.gnome.org/Git/CommitMessages

Nice work!

::: src/mainWindow.js
@@ -190,3 @@
             },
             'zoom-in': {
-                accels: ['<Primary>plus'],

Looks nice! I think tho that maybe we can keep both? Other applications seem to do that.
So this would become:

accels: ['<Primary>plus', 'plus'], ...

and same for minus. What do you think?

Okay. I will work upon the commit message. And regarding the latter one, i think we should discuss it with andreasn.He may help us better.

Adding Adreas to cc.

In order to keep consistency with Terminal, Evince, Documents, Nautilus etc. keeping ctrl++ and ctrl+- in addition to just + and - is probably sane.
Otherwise we'll end up with the situation where someone who's used to those apps tries to do that in Maps too and goes "Huh? Why didn't the regular zoom controls that I know from other apps work here? Is zoom broken?"

Created attachment 314306 [details] [review]
Using plus and minus along with ctrl++ and ctr-l- for zoom in and zoom out purpose

Review of attachment 314306 [details] [review]:

Awesome! Thanks!

It seems that the commit message got a bit mangled?

Could you send a new version with a cleaner commit message? Seem you forgot some new-lines?


For ease of user,both
 plus/minus and ctrl+/ctrl- are kept as keyboard shortcut for zoom-in/zoom-out
 purpose

Maybe just: "Added zoom accels for plus and minus." as commit message body?
The subject looks fine!

::: src/mainWindow.js
@@ +194,3 @@
             },
             'zoom-out': {
+                accels: ['minus', '<Ctrl>minus', 'KP_Subtract', '<Ctrl>KP_Subtract'],

Why the change from <Primary> to <Ctrl>?

Created attachment 314328 [details] [review]
Using plus and minus along with ctrl++ and ctrl-- for zoom in and zoom out purpose

(In reply to Jonas Danielsson from comment #11)
> Review of attachment 314306 [details] [review] [review]:
> 
> Awesome! Thanks!
> 
> It seems that the commit message got a bit mangled?
> 
> Could you send a new version with a cleaner commit message? Seem you forgot
> some new-lines?
> 
> 
> For ease of user,both
>  plus/minus and ctrl+/ctrl- are kept as keyboard shortcut for
> zoom-in/zoom-out
>  purpose
> 
> Maybe just: "Added zoom accels for plus and minus." as commit message body?
> The subject looks fine!
> 
> ::: src/mainWindow.js
> @@ +194,3 @@
>              },
>              'zoom-out': {
> +                accels: ['minus', '<Ctrl>minus', 'KP_Subtract',
> '<Ctrl>KP_Subtract'],
> 
> Why the change from <Primary> to <Ctrl>?

Actually I followed that one from evince code.Anyways I have made it to primary only in the latest patch and that's working fine.

Created attachment 314337 [details] [review]
MainWindow: Add simpler zoom accels

Added zoom accels for plus and minus

Review of attachment 314328 [details] [review]:

Thanks!

The code looks fine, I did however fixup your commit message, please think about that in the feature.
The objective is that it should look good and informative when you go

git log --oneline

Okay.Will keep that in mind from next time. Thanks for review.

Review of attachment 314337 [details] [review]:

pushed

(In reply to Jonas Danielsson from comment #15)
> Review of attachment 314328 [details] [review] [review]:
> 
> Thanks!
> 
> The code looks fine, I did however fixup your commit message, please think
> about that in the feature.
> The objective is that it should look good and informative when you go
> 
> git log --oneline

If you follow this guide, you'll be all set: https://wiki.gnome.org/Git/CommitMessages (Checklist is useful when you're about to send your patches and want to ensure everything is in order).

Seems = and Ctrl+= still don't work. We really should support '=' cause on English layouts you need to press shift+= for '+' key and that's very annoying and hence is probably the reason why other map implementations support '=' for zooming-in.

Created attachment 325492 [details] [review]
Add zoom in accel for equal

I know it's a very simple thing, but I wanted to get some more familiarity with how keyboard shortcuts are handled, I hope it's alright.

Review of attachment 325492 [details] [review]:

::: data/ui/help-overlay.ui
@@ +55,3 @@
                 <property name="visible">1</property>
                 <property name="title" translatable="yes" context="shortcut window">Zoom in</property>
+                <property name="accelerator">plus equal</property>

Is this the right way of doing it? How does evince look for instance? Do we have one row or two?

Created attachment 326426 [details]
Help overlay with equal accel

This is what it looks like. I followed the example of the Show Shortcuts entry and I think it's alright.