Closed Bug 834981 Opened 13 years ago Closed 12 years ago

Update Package App Stub Update UI/X

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::System
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.1 fixed)

Status:
RESOLVED FIXED
Project Flags:
blocking-b2g tef+
Tracking Flags:
Tracking Status
b2g18 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: nick, Assigned: nick)

References

Details

(Keywords: late-l10n, Whiteboard: QARegressExclude)

Attachments

(1 file, 11 obsolete files)

Bug 834981 Update Package App Stub Update UI/X
68.29 KB, patch
Pike
: review+
Details | Diff | Splinter Review
Update user flow for packaged app stubs. Stub automatically checks for update, reports if there is no connection otherwise reports update filesize, user may chose to click update, update is applied and app is closed.
Updated icon as per :fabrice
Attachment #706694 - Attachment is obsolete: true
Attachment #706694 - Flags: review?(fabrice)
Attachment #707155 - Flags: review?(fabrice)
Comment on attachment 707155 [details] [diff] [review] Bug 834981 - Update Package App Stub Update UI/X Forgot to freshen zip
Attachment #707155 - Flags: review?(fabrice)
freshened zip
Attachment #707155 - Attachment is obsolete: true
Attachment #707166 - Flags: review?(fabrice)
include binary modifications
Attachment #707166 - Attachment is obsolete: true
Attachment #707166 - Flags: review?(fabrice)
Attachment #707177 - Flags: review?(fabrice)
Added notification that we were checking for updates. This helps when the check isn't immediate.
Attachment #707177 - Attachment is obsolete: true
Attachment #707177 - Flags: review?(fabrice)
Attachment #707782 - Flags: review?(fabrice)
Attachment #707782 - Flags: feedback?(myk)
Comment on attachment 707782 [details] [diff] [review] Bug 834981 Update Package App Stub Update UI/X This looks great! It gives the right set of visual clues to stub and system activity throughout the update process.
Attachment #707782 - Flags: feedback?(myk) → feedback+
Blocks: 834794
blocking-b2g: --- → tef?
Comment on attachment 707782 [details] [diff] [review] Bug 834981 Update Package App Stub Update UI/X Review of attachment 707782 [details] [diff] [review]: ----------------------------------------------------------------- feedback+ from me, but this must really land with l10n from day 1. ::: external-dogfood-apps/packstubtest/index.html @@ +9,4 @@ > <body> > + <div id="main"> > + <img src="style/icons/icon.png" alt="logo"> > + <h3 class="center">Packaged Stub Test 0.1</h3> I would not hardcode a version number here, but it's up to you.
Attachment #707782 - Flags: review?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #9) > Comment on attachment 707782 [details] [diff] [review] > Bug 834981 Update Package App Stub Update UI/X > > Review of attachment 707782 [details] [diff] [review]: > ----------------------------------------------------------------- > > feedback+ from me, but this must really land with l10n from day 1. Thanks for the feedback. Should I close this bug and just add to my current working patch then? https://bugzilla.mozilla.org/show_bug.cgi?id=836507
(In reply to Nick Desaulniers [:\n] from comment #10) > (In reply to Fabrice Desré [:fabrice] from comment #9) > > Comment on attachment 707782 [details] [diff] [review] > > Bug 834981 Update Package App Stub Update UI/X > > > > Review of attachment 707782 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > feedback+ from me, but this must really land with l10n from day 1. > > Thanks for the feedback. Should I close this bug and just add to my current > working patch then? https://bugzilla.mozilla.org/show_bug.cgi?id=836507 As in should I close Bug 836507?
I would close 836507, yes
No longer blocks: 836507
Keywords: late-l10n
Comments for triage: This is required to implement the stubs for preinstalled apps. And it obviously to be localized.
If it's required for stubs, and those are a v1.0 requirement, we must block here.
Assignee: nobody → ndesaulniers
blocking-b2g: tef? → tef+
Bug 834981 - Update Package App Stub Update UI/X (localized)
Attachment #707782 - Attachment is obsolete: true
Attachment #708792 - Flags: review?(fabrice)
Attachment #708792 - Flags: feedback?(myk)
Comment on attachment 708792 [details] [diff] [review] Bug 834981 - Update Package App Stub Update UI/X (localized) Adding :stas to do the l10n portion of the review
Attachment #708792 - Flags: review?(stas)
Comment on attachment 708792 [details] [diff] [review] Bug 834981 - Update Package App Stub Update UI/X (localized) Review of attachment 708792 [details] [diff] [review]: ----------------------------------------------------------------- ::: external-dogfood-apps/packstubtest/js/app.js @@ +10,5 @@ > + p.setAttribute('class', 'center red'); > + p.appendChild(document.createTextNode(message)); > + $('checking').style.display = 'none'; > + $('main').appendChild(p); > + }, localeText = function localeText (localeKey, interObject) { Nit: put localeText declaration on its own line, like the other variable declarations. Also, nit: here and elsewhere, per Firefox code conventions, remove space between function name and opening parenthesis. @@ +39,5 @@ > + self.onprogress = function selfOnProgress () { > + // bug in marketplace > + // https://bugzilla.mozilla.org/show_bug.cgi?id=834863 > + progressBar.setAttribute('value', > + self.progress < self.downloadSize ? self.progress : self.downloadSize); The comment suggests that we have this conditional because of that bug in the Marketplace, but even once that bug is fixed (or if it never existed in the first place), we would still want to guard against progress exceeding downloadSize (for any download from any site). So the conditional isn't really Marketplace- or bug 834863-specific. @@ +44,4 @@ > }; > > + self.ondownloadsuccess = function selfOnDownloadSuccess () { > + window.setTimeout(window.close.bind(window), 1500); It'd be useful to say something at this point, like "Quitting app to apply update. Restart app to use it!" and then pause a few seconds before quitting to give the user a chance to read the message and understand it.
Attachment #708792 - Flags: feedback?(myk) → feedback+
This incorporates myk's suggestions from https://bugzilla.mozilla.org/show_bug.cgi?id=834981#c18
Attachment #708792 - Attachment is obsolete: true
Attachment #708792 - Flags: review?(stas)
Attachment #708792 - Flags: review?(fabrice)
Attachment #709099 - Flags: review?(fabrice)
Attachment #709099 - Flags: feedback?(myk)
Attachment #709099 - Flags: review?(stas)
Attachment #709099 - Flags: feedback?(myk) → feedback+
What are the next steps here? The deadline for landing blocker bugs is rapidly approaching (2/15) and getting this landed sooner rather than later to shake out any possible regressions would be ideal.
(In reply to Lukas Blakk [:lsblakk] from comment #20) > What are the next steps here? The deadline for landing blocker bugs is > rapidly approaching (2/15) and getting this landed sooner rather than later > to shake out any possible regressions would be ideal. It's waiting on review - it needs someone to do the Gaia portion of the review and someone else to do the l10n review.
Comment on attachment 709099 [details] [diff] [review] Bug 834981 Update Package App Stub Update UI/X - with myk's feedback Review of attachment 709099 [details] [diff] [review]: ----------------------------------------------------------------- I have no time to review that in the coming days.
Attachment #709099 - Flags: review?(fabrice)
Comment on attachment 709099 [details] [diff] [review] Bug 834981 Update Package App Stub Update UI/X - with myk's feedback Vivien - can you help do review here on this for the packaged app stub?
Attachment #709099 - Flags: review?(21)
Comment on attachment 709099 [details] [diff] [review] Bug 834981 Update Package App Stub Update UI/X - with myk's feedback Review of attachment 709099 [details] [diff] [review]: ----------------------------------------------------------------- r- mostly because i want to understand the reasoning beyond the l10n.js copy. ::: external-dogfood-apps/packstubtest/index.html @@ +28,2 @@ > <div id="console"></div> > + <script src="js/l10n.js"></script> Why do you need to duplicate the file? Does the shared/ build magic does not work for the external-dogfood-apps? If not I feel like we want to make it works instead of duplicating files. Or maybe one of your goal is to have a completely independent app? @@ +28,3 @@ > <div id="console"></div> > + <script src="js/l10n.js"></script> > + <script src="js/app.js"></script> Also why don't you move your js files at the top of the document with a defer attribute? ::: external-dogfood-apps/packstubtest/locales/packstubtest.en-US.properties @@ +1,3 @@ > +updateRequired=An update is needed for using this app > +updateCheck=Checking for update > +updateSize=An update is available, size: {{numBytes}}B There is an helper (FileSizeFormatter) in apps/settings/js/utils.js. Maybe we need to move it to shared/? @@ +4,5 @@ > +updateButton=Update > +failedFetch=Could not fetch update at this time > +noUpdate=No update available > +noInternet=Internet connection required > +closeWindow=Closing app to finish update. Please restart app. \ No newline at end of file Extra spaces between 'update.' and 'Please'. ::: external-dogfood-apps/packstubtest/style/app.css @@ +9,5 @@ > +body { > + height: 100%; > + background: darkgrey; > + text-shadow: 5px 5px 5px dimgrey; > +} nit: let's move the 'body' rule declaration at the top of the document. It seems more logical. @@ +25,5 @@ > + border-radius: 5px; > + box-shadow: 5px 5px 5px dimgrey; > + background: lightgrey; > + padding: 40px 0; > +} nit: since this is already an id you can remove the |div| part of the identifier. @@ +29,5 @@ > +} > + > +div#notifications { > + display: none; > +} nit: since this is already an id you can remove the |div| part of the identifier. @@ +31,5 @@ > +div#notifications { > + display: none; > +} > + > +div#checking p.center img { nit: since this is already an id you can remove the |div| part of the identifier.
Attachment #709099 - Flags: review?(21) → review-
Comment on attachment 709099 [details] [diff] [review] Bug 834981 Update Package App Stub Update UI/X - with myk's feedback Review of attachment 709099 [details] [diff] [review]: ----------------------------------------------------------------- Neither a strong r- nor r+ at this point. I'm wondering if we need the "data charges" disclaimer down there, but maybe not. Is there anything inside the application.zip that we'd need to look in to? Feel free to re-request review on the final patch. ::: external-dogfood-apps/packstubtest/locales/packstubtest.en-US.properties @@ +3,5 @@ > +updateSize=An update is available, size: {{numBytes}}B > +updateButton=Update > +failedFetch=Could not fetch update at this time > +noUpdate=No update available > +noInternet=Internet connection required Does this need a string about "data charges may apply"? I've seen that added in other places that talk about a required connection.
Attachment #709099 - Flags: review?(stas)
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #24) > Why do you need to duplicate the file? Does the shared/ build magic does not > work for the external-dogfood-apps? If not I feel like we want to make it > works instead of duplicating files. > > Or maybe one of your goal is to have a completely independent app? > > There is an helper (FileSizeFormatter) in apps/settings/js/utils.js. Maybe > we need to move it to shared/? The external-dogfood-apps do not have access to shared assets. I double checked for sanity. Shipping packaged app stubs should take priority over access to shared assets for third party apps, but there certainly is interest. See http://buildingfirefoxos.com/ (In reply to Axel Hetch (:pike) from comment #25) > Is there anything inside the application.zip that we'd need to look in to? Sanity check, make sure contents of zip match the actual source files, meaning I didn't forget to freshen the zip. If you do `zip -f application.zip` and see anything change, then that means the zip submitted with this patch is out of date.
Also, added the ability to recheck for updates on error.
Attachment #709099 - Attachment is obsolete: true
Attachment #712521 - Flags: review?(21)
Attachment #712521 - Flags: feedback?(myk)
Attachment #712521 - Flags: review?(l10n)
(In reply to Nick Desaulniers [:\n] from comment #26) > (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #24) > > Why do you need to duplicate the file? Does the shared/ build magic does not > > work for the external-dogfood-apps? If not I feel like we want to make it > > works instead of duplicating files. > > > > Or maybe one of your goal is to have a completely independent app? > > > > There is an helper (FileSizeFormatter) in apps/settings/js/utils.js. Maybe > > we need to move it to shared/? > > The external-dogfood-apps do not have access to shared assets. I double > checked for sanity. Shipping packaged app stubs should take priority over > access to shared assets for third party apps, but there certainly is > interest. See http://buildingfirefoxos.com/ The "shared" assets are only shared in the source repository. Each gaia app gets them in its zip at build time. So the question is if this is working for external-apps, and if not, why?
Can you attach the resulting application.zip? Or push a branch to your gaia clone so that I can pick it up there?
(In reply to Axel Hecht [:Pike] from comment #29) > Can you attach the resulting application.zip? Or push a branch to your gaia > clone so that I can pick it up there? Axel, you can apply this patch on top of a clone of gaia. That would prevent a separate attachment from becoming out of sync with the binary diff in the patch. Ping me on IRC if you would prefer a dummy branch in my gaia. It would be faster to DL and apply my patch if you have an existing clone then to fully clone a custom branch of mine, but I'm happy to do so if you prefer. `cd gaia` `curl -O https://bug834981.bugzilla.mozilla.org/attachment.cgi?id=712521 patch.txt` `git apply patch.txt`
My point is, I don't have to break my local clone if you push your branch to your clone, and instead just download the application.zip from there. Compared to checkout a branch, apply your patch, unzip the file somewhere and clean up the mess again.
(In reply to Axel Hecht [:Pike] from comment #31) > My point is, I don't have to break my local clone if you push your branch to > your clone, and instead just download the application.zip from there. > Compared to checkout a branch, apply your patch, unzip the file somewhere > and clean up the mess again. https://github.com/nickdesaulniers/gaia/tree/dummyBranch
Comment on attachment 712521 [details] [diff] [review] Bug 834981 - Update Package App Stub Update UI/X updated as per feedback of Myk, Vivien, and Axel Thanks. Sadly this confirms what I was afraid I'd find. The zip contains the en-US files and the locales.ini file. We'll need to get application.zip built as part of the profile build step. We can't localize the pre-packaged zip as is. I don't really have context as to what the application.zip does or is used for, nor what we're doing here in the big picture. Thus I don't really know how this works out.
Attachment #712521 - Flags: review?(l10n) → review-
(In reply to Axel Hecht [:Pike] from comment #33) > Comment on attachment 712521 [details] [diff] [review] > Bug 834981 - Update Package App Stub Update UI/X updated as per feedback of > Myk, Vivien, and Axel > > Thanks. Sadly this confirms what I was afraid I'd find. The zip contains the > en-US files and the locales.ini file. > > We'll need to get application.zip built as part of the profile build step. > We can't localize the pre-packaged zip as is. > > I don't really have context as to what the application.zip does or is used > for, nor what we're doing here in the big picture. Thus I don't really know > how this works out. I believe this is what Fabrice had mentioned, but I don't entirely understand. The application.zip needs to be built with gaia? I was just freshening it, then rebuiling the profile and opening the simulator with that profile to test it, or just loading that custom profile on device. Does the zipping of assets need to be a step in the gaia build process and it is currently not? But the application.zip was there when I started modifying it so maybe Myk knows if it was already part of the build process?
(In reply to Nick Desaulniers [:\n] from comment #26) > (In reply to Axel Hetch (:pike) from comment #25) > > Is there anything inside the application.zip that we'd need to look in to? > > Sanity check, make sure contents of zip match the actual source files, > meaning I didn't forget to freshen the zip. If you do `zip -f > application.zip` and see anything change, then that means the zip submitted > with this patch is out of date. Unfortunately, this won't work; instead, `zip -f` will freshen every file in the package; because Git doesn't preserve modification timestamps <https://git.wiki.kernel.org/index.php/GitFaq#Why_isn.27t_Git_preserving_modification_time_on_files.3F>; so zip will see them as updated files every time you check them out. However, to answer the original question, Axel: no, there's nothing in application.zip you need to review, since all the files in that package are present in unpackaged form in the external-dogfood-apps/packstubtest/ directory. (In reply to Fabrice Desré [:fabrice] from comment #28) > (In reply to Nick Desaulniers [:\n] from comment #26) > > The external-dogfood-apps do not have access to shared assets. I double > > checked for sanity. Shipping packaged app stubs should take priority over > > access to shared assets for third party apps, but there certainly is > > interest. See http://buildingfirefoxos.com/ > > The "shared" assets are only shared in the source repository. Each gaia app > gets them in its zip at build time. So the question is if this is working > for external-apps, and if not, why? It doesn't work for external apps because the Gaia build scripts don't package external apps, they just copy existing packages into the profile, per <https://github.com/mozilla-b2g/gaia/blob/4973f952862d1ec6a18ce430201160dcb7a84d93/build/webapp-manifests.js#L112>. Fabrice: you implemented that behavior in bug 812198. And that's how it was designed to work, according to the comments in that bug. Which is why my original patch contains an application.zip package, as does Nick's followup patch in this bug. And that package must contain all the files necessary to run the app, because the build scripts will only copy it to the profile. They won't modify it in any way. Perhaps the confusion here is because external-dogfood-apps/packstubtest/ also contains unpackaged copies of those files. I did that to make it easier to review those files. And also because we need to store them in some repository in order to develop and maintain them, so we might as well store them as close as possible to the place they're being used. But that doesn't mean Gaia's build scripts will package them. They won't. We have to do that ourselves. (In reply to Axel Hecht [:Pike] from comment #33) > We'll need to get application.zip built as part of the profile build step. > We can't localize the pre-packaged zip as is. Can you localize the unpackaged copies of the files? I.e.: external-dogfood-apps/packstubtest/js/l10n.js external-dogfood-apps/packstubtest/locales/packstubtest.en-US.properties If you can do that and provide us with the localizations, then we can generate a new application.zip that contains the localizations and commit that to the tree. If you can't do that, then we might be able to figure out how to build application.zip as part of the profile build step for this app, but that would certainly be a longer, more arduous, and riskier proposition. > I don't really have context as to what the application.zip does or is used > for, nor what we're doing here in the big picture. Thus I don't really know > how this works out. I hope I have now clarified matters sufficiently, but please let me know if there's anything still unclear!
The way we provide localizations is for them to be picked up at profile build time. You're welcome to tap in to that setup, but we can't provide yet another localization target, sorry. Localizing packaged apps is out of scope for us.
(In reply to Axel Hecht [:Pike] from comment #36) > The way we provide localizations is for them to be picked up at profile > build time. You're welcome to tap in to that setup, but we can't provide yet > another localization target, sorry. I'm not sure what you mean by "another localization target, but we aren't asking you to take responsibility for the build process, which will indeed be different from the standard Gaia process. We will handle that. The question at hand is whether, given a locale file in the Gaia repository at external-dogfood-apps/packstubtest/locales/packstubtest.en-US.properties, the localizers can create localizations and put them in a location that is accessible to us, like wherever localizations for other Gaia apps are currently stored (a branch/fork of the Gaia repository, a different repository, Transifex, etc.). If that is possible, then the localization strategy employed by this patch is feasible: we commit that file to the Gaia repository, the Gaia localizers use their existing process to localize it, and then we take the output of that process and build stub apps as needed using the stub app template. > Localizing packaged apps is out of scope for us. Erm, to make sure we're on the same page: we're not asking localizers to localize packaged apps; we're only asking them to localize the stub app template.
... and you think this is going to be less brittle and faster than creating a working build process? Another problem is that different partners will ship different sets of locales, which a single package won't help with? The l10n data is currently in http://hg.mozilla.org/gaia-l10n/, with a repo per locale. We may or may not need branches (as in different data sets) for branches, that's TBD.
(In reply to Axel Hecht [:Pike] from comment #38) > ... and you think this is going to be less brittle and faster than creating > a working build process? If the Gaia build process created a package from the source files in external-dogfood-apps/packstubtest/, it would still only produce a package for the template itself. And we would still have to create packages for each of the actual stub apps we intend to ship for a given Gaia release. Which apps cannot be in the Gaia repository, because information about those apps is confidential. So making the Gaia build process package the template wouldn't help us very much, if at all. We would still need to employ a separate build process to create the actual packages we will ship. > Another problem is that different partners will ship different sets of > locales, which a single package won't help with? Exactly. For each stub app we intend to ship for a given Gaia release, we need to create a package that contains app-specific information (name, icon) as well as locale-specific information (localized strings) for the specific set of locales in the Gaia release. Thus the build/packaging issues are a distraction. The question at hand is how to supply localizers with a set of strings to localize such that they can localize them and put their localizations in a location that is accessible to those of us responsible for the build/packaging of the individual stub apps. > The l10n data is currently in http://hg.mozilla.org/gaia-l10n/, with a repo > per locale. We may or may not need branches (as in different data sets) for > branches, that's TBD. Aha, thanks. So if we commit external-dogfood-apps/packstubtest/locales/packstubtest.en-US.properties to the Gaia repository, then can the localizers add localized versions of that file to the gaia-l10n repository?
I totally don't understand what you're doing, even remotely, sorry. I can't make any educated comment on what's going to happen, as I'm completely lost. Sure, you can put files in X, and you may get localized copies in Y, but if they're making any sense, no idea.
(In reply to Axel Hecht [:Pike] from comment #40) > I totally don't understand what you're doing, even remotely, sorry. I can't > make any educated comment on what's going to happen, as I'm completely lost. Would it help to chat via Vidyo?
Comment on attachment 712521 [details] [diff] [review] Bug 834981 - Update Package App Stub Update UI/X updated as per feedback of Myk, Vivien, and Axel Just got off a call with Myk and Nick. We should still do the update-size bits. With that, r=me, knowing the following: Making sure that the localized strings show up in the non-generic stubs is Nick's problem. We'll not test the localized strings that would be in the generic stub, as it's an edge case, it's not part of standard builds, it's hard to expose all strings even inside the generic stub, and it's in a bad-UX context already. Thus, we also don't need to bother updating the application.zip for updates to locales.
Attachment #712521 - Flags: review- → review+
Comment on attachment 712521 [details] [diff] [review] Bug 834981 - Update Package App Stub Update UI/X updated as per feedback of Myk, Vivien, and Axel As with previous feedback in comment 18, this still looks good from a general architectural perspective.
Attachment #712521 - Flags: feedback?(myk) → feedback+
Added formatSizeFormater
Attachment #712521 - Attachment is obsolete: true
Attachment #712521 - Flags: review?(21)
Attachment #713143 - Flags: review?(21)
Attachment #713143 - Flags: feedback?(myk)
Attachment #713143 - Flags: review?(l10n)
Sorry guys, I had some inconsistencies with the use of spacing in my prior patch.
Attachment #713143 - Attachment is obsolete: true
Attachment #713143 - Flags: review?(l10n)
Attachment #713143 - Flags: review?(21)
Attachment #713143 - Flags: feedback?(myk)
Attachment #713166 - Flags: review?(21)
Attachment #713166 - Flags: feedback?(myk)
Attachment #713166 - Flags: review?(l10n)
Comment on attachment 713166 [details] [diff] [review] Bug 834981 Update Package App Stub Update UI/X Review of attachment 713166 [details] [diff] [review]: ----------------------------------------------------------------- Just to be sure, the hardcoded strings in the html are part of the packaging for the actual app this is stubbing? I don't think the number format stuff is there just yet. We'll need something like _('updateSize' {amount: fixedFloat, unit: _(units[k])}) with both the units being localized, as well as the potential space and number/unit ordering by having the localized string be # LOCALIZATION_NOTE(updateSize): amount is a fixed precision float, unit is localized value of KB etc updateSize=An update of {{amount}} {{unit}} is available. Data charges may apply. KB=KB MB=MB ... ::: external-dogfood-apps/packstubtest/index.html @@ +3,4 @@ > <head> > <meta charset="utf-8"> > <meta http-equiv="pragma" content="no-cache"> > <title>Packaged Stub Test</title> Is the title and the <h3> part of the branding that's gonna happen when actually creating a real stub? ::: external-dogfood-apps/packstubtest/js/app.js @@ +21,5 @@ > + errors.appendChild(button); > + }, > + fileSizeFormat = function fileSizeFormat(sizeInBytes) { > + var i = 0, units = [ > + 'B', 'KB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB', 'YB' Localizers actually want to localize the units, too. @@ +28,5 @@ > + return { size: 0, unit: 'B' }; > + } > + for (; sizeInBytes >= 1024; sizeInBytes /= 1024, i++); > + return parseFloat(sizeInBytes.toFixed(i > 0 ? 2 : 0)).toString() + > + ' ' + (i < units.length ? units[i] : ''); You're hardcoding a space between number and unit here? That's not good for all languages.
Attachment #713166 - Flags: review?(l10n) → review-
Comment on attachment 713166 [details] [diff] [review] Bug 834981 Update Package App Stub Update UI/X Review of attachment 713166 [details] [diff] [review]: ----------------------------------------------------------------- I really don't like the fact that the l10n.js file is duplicated but I don't have a better aternative now so let's land that. ::: external-dogfood-apps/packstubtest/js/app.js @@ +1,1 @@ > +window.addEventListener('localized', function localized() { The l10n.js file is out of date. They use onready now. @@ +32,5 @@ > + ' ' + (i < units.length ? units[i] : ''); > + }, > + getSelfCheckUpdate = function getSelfCheckUpdate() { > + var getSelf = navigator.mozApps.getSelf(); > + getSelf.onsuccess = function getSelfOnSuccess() { nit: the indent seems broken.
Attachment #713166 - Flags: review?(21) → review+
(In reply to Axel Hecht [:Pike] from comment #46) > Comment on attachment 713166 [details] [diff] [review] > Bug 834981 Update Package App Stub Update UI/X > > Review of attachment 713166 [details] [diff] [review]: > ----------------------------------------------------------------- > > Just to be sure, the hardcoded strings in the html are part of the packaging > for the actual app this is stubbing? > Is the title and the <h3> part of the branding that's gonna happen when > actually creating a real stub? Yes. > I don't think the number format stuff is there just yet. > > We'll need something like > > _('updateSize' {amount: fixedFloat, unit: _(units[k])}) > > with both the units being localized, as well as the potential space and > number/unit ordering by having the localized string be > > # LOCALIZATION_NOTE(updateSize): amount is a fixed precision float, unit is > localized value of KB etc > updateSize=An update of {{amount}} {{unit}} is available. Data charges may > apply. > KB=KB > MB=MB > ... > > ::: external-dogfood-apps/packstubtest/js/app.js > @@ +21,5 @@ > > + errors.appendChild(button); > > + }, > > + fileSizeFormat = function fileSizeFormat(sizeInBytes) { > > + var i = 0, units = [ > > + 'B', 'KB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB', 'YB' > > Localizers actually want to localize the units, too. Ok, the string literals here will stay the same, but I'll use them as keys to call into the l10n stuff. > @@ +28,5 @@ > > + return { size: 0, unit: 'B' }; > > + } > > + for (; sizeInBytes >= 1024; sizeInBytes /= 1024, i++); > > + return parseFloat(sizeInBytes.toFixed(i > 0 ? 2 : 0)).toString() + > > + ' ' + (i < units.length ? units[i] : ''); > > You're hardcoding a space between number and unit here? That's not good for > all languages. Yeah I just realized this method is all sorts of messed up. Can return an anonymous object literal or a string. Bad! (In reply to Vivien Nicolas [:vingetun][:21] from comment #47) > Comment on attachment 713166 [details] [diff] [review] > Bug 834981 Update Package App Stub Update UI/X > > Review of attachment 713166 [details] [diff] [review]: > ----------------------------------------------------------------- > The l10n.js file is out of date. They use onready now. > nit: the indent seems broken. Ok thanks for pointing these out, will fix.
Vivien, I've grabbed webl10n from github, and they have not switched from the localized event.
Attachment #713166 - Attachment is obsolete: true
Attachment #713166 - Flags: feedback?(myk)
Attachment #713607 - Flags: review?(21)
Attachment #713607 - Flags: feedback?(myk)
Attachment #713607 - Flags: review?(l10n)
Attachment #713607 - Flags: review?(jcarpenter)
Comment on attachment 713607 [details] [diff] [review] Bug 834981 Update Package App Stub Update UI/X This doesn't need my feedback, as the changes from previous patches aren't significant enough for me to change my mind about it. :-)
Attachment #713607 - Flags: feedback?(myk)
Hi Nick, sorry to be a pain,but would you mind calling out the specifics of what needs review, and/or add a screenshot of the implementation? I had a look at the patch but I'm having trouble discerning the current state from it (especially at 3am ;)
Comment on attachment 713607 [details] [diff] [review] Bug 834981 Update Package App Stub Update UI/X Review of attachment 713607 [details] [diff] [review]: ----------------------------------------------------------------- With the nit about way-too-big download sizes, r=me. ::: external-dogfood-apps/packstubtest/js/app.js @@ +26,5 @@ > + ]; > + if (!sizeInBytes || sizeInBytes < 0) { > + return { amount: 0, unit: 'B' }; > + } > + for (; sizeInBytes >= 1024; sizeInBytes /= 1024, i++); I think this loop should ensure that i<units.length, so that ... @@ +29,5 @@ > + } > + for (; sizeInBytes >= 1024; sizeInBytes /= 1024, i++); > + return { > + amount: parseFloat(sizeInBytes.toFixed(i > 0 ? 2 : 0)), > + unit: l10n.get(i < units.length ? units[i] : 'B') ... this conditional doesn't end up doing 15B for some 10^28B. Just theory. Also, I'd stop the units at GB, nobody is going to say yes to 12YB of update size.
Attachment #713607 - Flags: review?(l10n) → review+
This remains a blocker for Mozilla and our partners, given this was a v1 requirement (the ability to provide a stub on the device).
Final updates as per :pike. With r+, :myk, may I just issue a pull request against gaia?
Attachment #713607 - Attachment is obsolete: true
Attachment #713607 - Flags: review?(jcarpenter)
Attachment #714141 - Flags: review?(l10n)
(In reply to Nick Desaulniers [:\n] from comment #54) > Created attachment 714141 [details] [diff] [review] > Bug 834981 Update Package App Stub Update UI/X > > Final updates as per :pike. With r+, :myk, may I just issue a pull request > against gaia? Since Pike granted review "with the nit about way-too-big download sizes", and vingtetun then granted review without further comment, you can carry forward their reviews as long as that's the only thing you changed. Which makes this ready for commission. So do prepare a Gaia pull request and then ask vingtetun to merge it for you!
Comment on attachment 714141 [details] [diff] [review] Bug 834981 Update Package App Stub Update UI/X Review of attachment 714141 [details] [diff] [review]: ----------------------------------------------------------------- what myk said ;-)
Attachment #714141 - Flags: review?(l10n) → review+
If it's r+, could you land it and mark it as RESOLVED/FIXED please ?
Flags: needinfo?(ndesaulniers)
I can't land code in gaia. My pull request from Comment 58 is still open.
Flags: needinfo?(ndesaulniers)
Nick, if you can not push yourself, add the chechin-needed keyword to the bug. https://github.com/mozilla-b2g/gaia/commit/5b61b63fd0127d9d25bddea4b026b9c8f72e3302 I'm not sure why this is is dogfood-apps however...
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
v1-train@09ed89f142a6446bf6c528ba6c70fd86873f5d3a v1.0.1@b3bb39b9296e2066051221941fd88679de236f8b
status-b2g18: affected → fixed
status-b2g18-v1.0.1: affected → fixed
Whiteboard: QARegressExclude
No Test case creation is needed in moztrap for this issue.
Flags: in-moztrap-
Cannot verify, need steps to blackbox test this issue.
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: