906323 - It should be possible to update a menuitem's label

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

From what I can tell:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1536
https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/NativeWindow/menu/update

It is not possible to update a menuitem's label text.  This should be possible.

If you want a use case take Greasefire, which shows how many userscripts are available for the page that is being viewed.  So the label is  "XYZ Scripts available".

Is there any platform issue with doing this? or was it just omitted for no reason?

Comment 1

[:Margaret Leibovic]

Yeah, this sounds like it was probably just an oversight. Sriram, is that right?

It seems like fixing this would be as straightforward as adding a case to update info.label in updateAddonMenuItem. We could probably also allow updating the icon as well.

Comment 2

[Sriram Ramasubramanian [:sriram]]

I remember that the decision was to change only the checked, checkable, enabled and visibility values. We could change title too -- not sure if we really want it. It's easy to do that though.

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #2)
> I remember that the decision was to change only the checked, checkable,
> enabled and visibility values. We could change title too -- not sure if we
> really want it. It's easy to do that though.

The reason that I'm asking for it is so that we can include the feature in our SDK menuitem implementation, if Fennec doesn't support it then we'll have to leave it out.  I can't imagine why any platform would not be able to support it tho, so I think we should include the feature.

The use case that I gave seems like one that would apply to Desktop as well as Mobile.

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

I can take this bug if everyone else is busy, I'd like to do more platform bugs anyhow.

Comment 5

[:Margaret Leibovic]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #4)
> I can take this bug if everyone else is busy, I'd like to do more platform
> bugs anyhow.

Go for it! Sriram and I can both help you out, and either one of us can review your patch.

Comment 6

[Mark Capella [:capella]]

Attachment: bug906323

Misc quickie...

Comment 7

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 817893 [details] [diff] [review]
bug906323

Drive-by: there is no need for try..catch. Please use optString() with a default value of "". All good :d

Comment 8

[Mark Capella [:capella]]

Attachment: bug906323

Ah, close! This will do what I had in mind ...

Comment 9

[Mark Capella [:capella]]

Using the test case ... I want the last item to stay "Last Try..."

  gMenu01 = aWindow.NativeWindow.menu.add({
    parent: gMenu00,
    name: "Kill Me",
    checkable: false,
    icon: gAddonData.resourceURI.spec + "lightblue2.png",
    callback: function() {
      if (gTitle == 0) {
        gTitle++;
        aWindow.NativeWindow.menu.update(gMenu01, { name: "Try Again!", checkable: true , checked: true } );
      } else {
        if (gTitle == 1) {
          gTitle++;
          aWindow.NativeWindow.menu.update(gMenu01, { name: "", checkable: false , checked: true } );
        } else {
          if (gTitle == 2) {
            gTitle++;
            aWindow.NativeWindow.menu.update(gMenu01, { name: "Last Try...", checkable: true, checked: false } );
          } else {
            aWindow.NativeWindow.menu.update(gMenu01, { checked: true } );     // <========== Don't blank me
          }
        }
      }
    }
  });

Comment 10

[:Margaret Leibovic]

Comment on attachment 817919 [details] [diff] [review]
bug906323

Passing this review off to Sriram, since he wrote this code.

However, it looks like we also need to add a similar line for cached menu items:
http://hg.mozilla.org/mozilla-central/annotate/c14ca6b27b30/mobile/android/base/BrowserApp.java#l1779

Comment 11

[Mark Capella [:capella]]

Attachment: bug906323 v2

Hey sriram ... margaret caught me again ... we need this too right?

Comment 12

[Mark Capella [:capella]]

TRY success! https://tbpl.mozilla.org/?tree=Try&rev=735e6ee06df9

Comment 13

[Mark Capella [:capella]]

Squishes a birthday bug :-D
https://hg.mozilla.org/integration/fx-team/rev/1674e4611f80

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1674e4611f80