Closed Bug 934177 Opened 6 years ago Closed 6 years ago

[App manager] Some strings are missing

Categories

(DevTools :: WebIDE, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: tchevalier, Assigned: boopathi)

Details

(Whiteboard: [good first bug][lang=html][mentor=jryans][qa-])

Attachments

(1 file, 5 obsolete files)

Some strings about the status of an App are not localizable, like:

"PACKAGED"
"HOSTED"
"CERTIFIED"
"VALID"
"WARNING"

+ The tooltip on permission table headers: Type:'certified', Type:'privileged', etc.
That's actually the result of a discussion on IRC (those are JSON snippets), so they don't really need to be translated. Does it make sense?

<jryans>: if you have a UI string that is not meant to be translated, should it be hardcoded in the UI, or still be broken out as a property, but with a localization note not to translate any of it?
<flod>: jryans: do you have an example?
<jryans> flod: in this patch: https://bugzilla.mozilla.org/attachment.cgi?id=823414&action=diff the lines like "device.plainTooltip" are just bits of JSON, so they wouldn't make sense to translate
<jryans> flod: just double-checking what i should recommend during my review
<flod>: in those tooltips I believe "type" should be localized, right?
<jryans>: flod: no, it's meant to match what would be in a manifest file: https://developer.mozilla.org/en-US/Apps/Developing/Manifest#type
<jryans>: flod: so it expects the key "type"
<Pike>: jryans: I suggest to keep them in code and not expose to l10n
<jryans>: okay, thanks!
<flod>: as it is, I would have translated the whole thing
<jryans>: okay, good to know :)
(In reply to Francesco Lodolo [:flod] from comment #1)
> That's actually the result of a discussion on IRC (those are JSON snippets),
> so they don't really need to be translated. Does it make sense?

But then I realized this answer only the last part of the question (tooltips).

If a locale is localizing "packaged", "hosted", etc. in the descriptions (I'm not) I guess it makes sense to have them localizable.
(In reply to Francesco Lodolo [:flod] from comment #2)
> (In reply to Francesco Lodolo [:flod] from comment #1)
> > That's actually the result of a discussion on IRC (those are JSON snippets),
> > so they don't really need to be translated. Does it make sense?
> 
> But then I realized this answer only the last part of the question
> (tooltips).

Ok for the tooltips. I didn't get it's a representation of the manifest file.

> 
> If a locale is localizing "packaged", "hosted", etc. in the descriptions
> (I'm not) I guess it makes sense to have them localizable.

Yes, for instance in French we localize those terms everywhere (MDN, Marketplace, websites, products).
This is not a major issue since developers will understand what a "packaged" application is, but it's better if we can be consistent.
Yes, let's keep the tooltips as-is, since they are JSON snippets.

Marking as a good first bug to expose the status words as translatable strings.
Whiteboard: [good first bug][lang=html][mentor=jryans]
Hi,

I am interested in working on this bug. So please can you assign this bug to me.

Thanks in Advance,

Regards,
A. Anup.
Hi Anup, thanks for your help!
Assignee: nobody → allamsetty.anup
Some strings has landed with bug 912918.

The only missing strings are now the type of application ("PACKAGED", "HOSTED", "CERTIFIED" -- I think that's all?)
Attached patch bug-912918.patch (obsolete) — Splinter Review
Attachment #8340968 - Flags: review?(jryans)
Hi, 

I created a path for this, please tell me if is correct.

Regards,
Gio
(In reply to Giovanny Andres Gongora Granada from comment #9)
> Hi, 
> 
> I created a path for this, please tell me if is correct.
> 
> Regards,
> Gio

Thanks for your patch, Gio!

Here are some thoughts

Did you check the entities are in the html page? I don't think they are. Here is an example of how to implement this https://bug912918.bugzilla.mozilla.org/attachment.cgi?id=828735

Another thing, you don't have to capitalize the strings, it will be handled in CSS, so you can just use "Packaged" etc.
Comment on attachment 8340968 [details] [diff] [review]
bug-912918.patch

Review of attachment 8340968 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Théo Chevalier [:tchevalier] from comment #10)
> (In reply to Giovanny Andres Gongora Granada from comment #9)
> > Hi, 
> > 
> > I created a path for this, please tell me if is correct.
> > 
> > Regards,
> > Gio
> 
> Thanks for your patch, Gio!
> 
> Here are some thoughts
> 
> Did you check the entities are in the html page? I don't think they are.
> Here is an example of how to implement this
> https://bug912918.bugzilla.mozilla.org/attachment.cgi?id=828735
> 
> Another thing, you don't have to capitalize the strings, it will be handled
> in CSS, so you can just use "Packaged" etc.

Yes, these are all good comments.  Gio, please take a look at these.

Also, have you been able to build Firefox with your patch locally to verify that it achieves the desired result?  You should always do that first before asking for review.  Check out our docs[1] for more info if you haven't done so yet.

Good luck!  If you have issues, you can also stop by our IRC room (#devtools) too.

[1]: https://wiki.mozilla.org/DevTools/Hacking
Attachment #8340968 - Flags: review?(jryans)
(In reply to Théo Chevalier [:tchevalier] from comment #10)
> Here are some thoughts
> 
> Did you check the entities are in the html page? I don't think they are.
> Here is an example of how to implement this
> https://bug912918.bugzilla.mozilla.org/attachment.cgi?id=828735

Is it necessary to check the entities in the html page? What is the use of that?
 
> Another thing, you don't have to capitalize the strings, it will be handled
> in CSS, so you can just use "Packaged" etc.

To test this are we supposed to clone the entire thing again by using integration/fx-team?
(In reply to Anup from comment #12)
> (In reply to Théo Chevalier [:tchevalier] from comment #10)
> > Here are some thoughts
> > 
> > Did you check the entities are in the html page? I don't think they are.
> > Here is an example of how to implement this
> > https://bug912918.bugzilla.mozilla.org/attachment.cgi?id=828735
> 
> Is it necessary to check the entities in the html page? What is the use of
> that?

New strings were added in the current patch on this bug, but it doesn't look like they are actually used anywhere.  HTML files need to be updated to reference the strings as well.  Look at the linked example.

> > Another thing, you don't have to capitalize the strings, it will be handled
> > in CSS, so you can just use "Packaged" etc.
> 
> To test this are we supposed to clone the entire thing again by using
> integration/fx-team?

You'll want to keep the source tree around for developing, testing, etc.  So yes, you'll need to clone again if you don't have it anymore...  You don't need two copies though.  For string changes, it should be enough to build Firefox and see that your string are used correctly.

For more info, see our hacking guide[1].

[1]: https://wiki.mozilla.org/DevTools/Hacking
where can we check the entities in the html pages? I mean where the file is located?
(In reply to Anup from comment #14)
> where can we check the entities in the html pages? I mean where the file is
> located?

Take a look at the HTML files in browser/devtools/app-manager/content.
Assignee: allamsetty.anup → boopathi
I  look this permission table(Tools menu -> Web Developer -> App Manager). And Only HOSTED is missing in heading section(Column). Is it necessary to add VALID, WARNINGS, PACKAGED columns also ?

What I found was there is need to edit app-manager.dtd and device.xhtml file. So is there any other files needed to modify(any javascripts and stylesheets).
Thank you.
(In reply to Boopathi K from comment #16)
> I  look this permission table(Tools menu -> Web Developer -> App Manager).
> And Only HOSTED is missing in heading section(Column). Is it necessary to
> add VALID, WARNINGS, PACKAGED columns also ?
> 
> What I found was there is need to edit app-manager.dtd and device.xhtml
> file. So is there any other files needed to modify(any javascripts and
> stylesheets).
> Thank you.

We talked more about this on IRC, but just to summarize here for others, there are no new columns needed.  We just need to make PACKAGED, HOSTED, and CERTIFIED from the top-right corner of the project view translatable.  To do so, I think it would be best to put all the values in the markup, like with valid / error / warning, and then show only the correct one with CSS.
Attached patch Patch for this bug. (obsolete) — Splinter Review
I have uploaded the patch, Review the patch. Included privileged, certified and web appType.
Flags: needinfo?
Comment on attachment 8362220 [details] [diff] [review]
Patch for this bug.

Hi, thanks for the patch.

When submitting the file, you can ask a review (?) and set a reviewer, don't set a NEEDINFO, especially without specifying an email ;-)
Attachment #8362220 - Flags: review?(jryans)
(In reply to Francesco Lodolo [:flod] from comment #19)
> Comment on attachment 8362220 [details] [diff] [review]
> Patch for this bug.
> 
> Hi, thanks for the patch.
> 
> When submitting the file, you can ask a review (?) and set a reviewer, don't
> set a NEEDINFO, especially without specifying an email ;-)

Sorry i didn't know that, and this is my first bug submission in bugzilla. Any way thanks for your information, i'll do it on next time.
Comment on attachment 8362220 [details] [diff] [review]
Patch for this bug.

Review of attachment 8362220 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!  Take a look at the issues I've outlined below and let me know if you have any questions.  Then you can address these issues, and attach a revised patch and r? me again.

For the patch commit message, it's better to attempt a short summary of what was actually changed, rather than just repeating the title of the bug.  Also, you should end the commit message with "r=jryans".  See the wiki[1] for more info.

In general, there are many lines with trailing whitespace.  Please configure your text editor to remove that for you, or find some other way of getting rid of it.

I had some issues applying the patch, but this is most likely because someone else has also made changes in the files you are changing.  For your next patch, be sure to base it on the latest changes in fx-team.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: browser/devtools/app-manager/content/projects.js
@@ +55,5 @@
>      this.template.start();
>  
>      AppProjects.load().then(() => {
>        AppProjects.store.object.projects.forEach(UI.validate);
> +      AppProjects.store.object.projects.forEach(UI.appType);

This is not needed, see comments below.

@@ +180,5 @@
>  
>        });
>  
>    },
> +  appType : function(project){

This function is not needed, since as I mention in my comments on the HTML file, the type is already accessible, so we don't need to get it again.

::: browser/devtools/app-manager/content/projects.xhtml
@@ +66,5 @@
>              <div class="project-status" template='{"type":"attribute","path":"validationStatus","name":"status"}'>
>                <p class="project-validation valid">&projects.valid;</p>
>                <p class="project-validation warning">&projects.warning;</p>
>                <p class="project-validation error">&projects.error;</p>
>                <p class="project-type" template='{"type":"textContent","path":"type"}'></p>

Seems like you need to remove this too, so you are not repeating the type...

@@ +67,5 @@
>                <p class="project-validation valid">&projects.valid;</p>
>                <p class="project-validation warning">&projects.warning;</p>
>                <p class="project-validation error">&projects.error;</p>
>                <p class="project-type" template='{"type":"textContent","path":"type"}'></p>
> +              <div class="project-app-Type" template='{"type":"attribute","path":"apptype","name":"apptype"}'>

This new div should live at the same level as the "project-status" element, not inside it.  Also, the class name should be lowercase only.  Once you've removed the line above, you should use the more succinct class "project-type" here.

Also, you have the right idea by storing the type as an attribute that you can use for styling via CSS.  But, you can just access the type value using "type" as the "path", since that what was already being done before (from the line above).  Naming the attribute "type" should also be fine.

@@ +68,5 @@
>                <p class="project-validation warning">&projects.warning;</p>
>                <p class="project-validation error">&projects.error;</p>
>                <p class="project-type" template='{"type":"textContent","path":"type"}'></p>
> +              <div class="project-app-Type" template='{"type":"attribute","path":"apptype","name":"apptype"}'>
> +                <p class="project-AppType privileged">&projects.privileged;</p>

The class "project-AppType" seems unnecessary.  You can just use the selector ".project-type p" to refer to all of these.

Also, this type should be called "packaged", like it is today.

@@ +70,5 @@
>                <p class="project-type" template='{"type":"textContent","path":"type"}'></p>
> +              <div class="project-app-Type" template='{"type":"attribute","path":"apptype","name":"apptype"}'>
> +                <p class="project-AppType privileged">&projects.privileged;</p>
> +                <p class="project-AppType certified">&projects.certified;</p>
> +                <p class="project-AppType web">&projects.web;</p>

This type should be called "hosted".

::: browser/locales/en-US/chrome/browser/devtools/app-manager.dtd
@@ +82,5 @@
>  <!ENTITY projects.manifestViewerTooltip "Examine your app's manifest in the panel below">
>  <!ENTITY projects.valid "Valid">
>  <!ENTITY projects.error "Error">
>  <!ENTITY projects.warning "Warning">
> +<!ENTITY projects.privileged "Privileged">

Change to "Packaged"

@@ +84,5 @@
>  <!ENTITY projects.error "Error">
>  <!ENTITY projects.warning "Warning">
> +<!ENTITY projects.privileged "Privileged">
> +<!ENTITY projects.certified "Certified">
> +<!ENTITY projects.web "Web">

Change to "Hosted"

::: browser/themes/shared/devtools/app-manager/projects.css
@@ +343,5 @@
> +  font-size: 10px; 
> +}
> +
> +[apptype="privileged"] > .project-AppType.privileged,
> +[apptype~="certified"] > .project-AppType.certified,

You only want =, not ~=, since there will only every be a single value in the attribute at a time.  The validation case above is more complex, since multiple labels are allowed to appear simultaneously.
Attachment #8362220 - Flags: review?(jryans) → review-
It looks like I made have confused myself early on and led you a bit astray, which explains some of things you added in the patch above.

There are really two "types" for a project in the App Manager: The there is the "project type", which has 2 values, "hosted" or "packaged", which is mostly an indication of where the project came from (a remote site or a folder on disk).  There is also the "manifest type", which has 3 values, "certified", "privileged", or "web".

The "project type" is what we were already exposing in the top right of the project / app view.  In your patch, you headed down the path of adding the "manifest type" as entirely new, additional information.  But this bug is not about that.  We are only trying to make *existing* information possible to translate, which means the "project type" is what we are worried about here.  Hopefully my comments make a bit more sense with this in mind.

Sorry for the initial confusion!
Attachment #8365521 - Flags: review?(jryans)
Comment on attachment 8365521 [details] [diff] [review]
The missing strings hosted and packaged are localized.

Review of attachment 8365521 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, this is much closer!  Just a few small things to clean up, and then this will be ready to go.

Also, when you attach an updated patch, you should mark the old ones "obsolete", which you can do from the "new attachment" screen.  I'll clean up the old one this time.

::: browser/devtools/app-manager/content/projects.xhtml
@@ +66,5 @@
>              <div class="project-status" template='{"type":"attribute","path":"validationStatus","name":"status"}'>
>                <p class="project-validation valid">&projects.valid;</p>
>                <p class="project-validation warning">&projects.warning;</p>
>                <p class="project-validation error">&projects.error;</p>
> +              <div class="project-status" template='{"type":"attribute","path":"type","name":"type"}'>

Move this block one level up, so that it is on the same level as the other div with class "project-status".  You may need to adjust CSS after to ensure the same appearance is maintained.  It's probably fine for this div to keep the class "project-status" once you've moved it, but double-check that to make sure that the styles this applies still make sense.

::: browser/themes/shared/devtools/app-manager/projects.css
@@ +337,5 @@
>    display: inline;
>  }
>  
> +.project-type {
> +   display: none;

Be sure to double check code style in patches.  There's a extra space at the beginning of the line here.

@@ +344,4 @@
>  
> +[type="hosted"] > .project-type.hosted,
> +[type="packaged"] > .project-type.packaged {
> +  display:inline;

Add a space after the colon.
Attachment #8365521 - Flags: review?(jryans)
Status: NEW → ASSIGNED
Attached patch Bug-934177.patch (obsolete) — Splinter Review
Patch Updated one.
Attachment #8365521 - Attachment is obsolete: true
Attachment #8375463 - Flags: review?(jryans)
Attached patch Bug-934177.patch (obsolete) — Splinter Review
Attachment #8375463 - Attachment is obsolete: true
Attachment #8375463 - Flags: review?(jryans)
Attachment #8375465 - Flags: review?(jryans)
Comment on attachment 8375465 [details] [diff] [review]
Bug-934177.patch

Review of attachment 8375465 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me!  A few places to clean up some trailing whitespace, but this should be ready to land after that.

After you make these changes, attach your updated patch, and you can carry over the r+ onto your new patch.  There are no tests to run for this change, so we don't need to push this patch to Try.  Then, you can set the "checkin-needed" keyword on this bug, and someone will land your patch for you.

::: browser/themes/shared/devtools/app-manager/projects.css
@@ +340,5 @@
> +.project-type {
> +  display: none;
> +  margin-left: 10px;
> +}
> + 

Remove the trailing whitespace here.

@@ +345,5 @@
> +[type="hosted"] > .project-type.hosted,
> +[type="packaged"] > .project-type.packaged {
> +  display: inline;
> +}
> + 

Remove the trailing whitespace here.
Attachment #8375465 - Flags: review?(jryans) → review+
Attached patch Bug934177.patchSplinter Review
Attachment #8375465 - Attachment is obsolete: true
Attachment #8377516 - Flags: review+
Attachment #8377516 - Flags: checkin?
Comment on attachment 8377516 [details] [diff] [review]
Bug934177.patch

Please just use the checkin-needed bug keyword in the future :)
Attachment #8377516 - Flags: checkin? → checkin+
https://hg.mozilla.org/integration/fx-team/rev/1ca7f4e8fd0f
Whiteboard: [good first bug][lang=html][mentor=jryans] → [good first bug][lang=html][mentor=jryans][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1ca7f4e8fd0f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=html][mentor=jryans][fixed-in-fx-team] → [good first bug][lang=html][mentor=jryans]
Target Milestone: --- → Firefox 30
Whiteboard: [good first bug][lang=html][mentor=jryans] → [good first bug][lang=html][mentor=jryans][qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.