Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2825153002:
Update CUPS settings UI to allow USB printers to be added via discovery. (Closed)
|
Created:
3 years, 8 months ago by Carlson Modified:
3 years, 7 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate CUPS settings UI to allow USB printers to be added via discovery.
This re-enables the "discovery" dialog when a user asks to add a
printer; the dialog is populated with any USB printers that could not
be set up automatically. (Eventually, this will also be populated
with auto-discovered network printers).
If the user selects a printer from the list, they are taken to the
manufacturer-model dialog to select a driver.
This change also alters a button in the ui. Right now, there's always
an "Add printer manually" button in the manufacturer-model dialog.
But this *actually* functions as a "back" button when adding a printer
manually, as it takes you back to the previous dialog with the values
you entered there, and, in the discovery case, this button doesn't
make sense as it doesn't have previous values to take you back to.
So the button has been re-texted with 'back', and now takes you to
the previous dialog, whether that was discovery or manual addition.
Finally, this change also fixes a few things in URI handling for
usb printers in cups_printers_handler so that, when we add a printer
from the manufacturer-model dialog, we get a correct URI to do so.
BUG=616866
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2825153002
Cr-Commit-Position: refs/heads/master@{#471858}
Committed: https://chromium.googlesource.com/chromium/src/+/2ed79b998d112e09fc22678cd4bd838a465ee569
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address xdai comments, rebase #Patch Set 3 : Remove extra copy code #
Total comments: 14
Patch Set 4 : Address xdai comments. #
Total comments: 2
Patch Set 5 : Single Canonical CupsPrinterInfo, consistent back button #Patch Set 6 : Update CUPS settings UI to allow USB printers to be added via discovery. #
Total comments: 18
Patch Set 7 : Address more xdai comments #Patch Set 8 : One more round of xdai comments. #
Total comments: 2
Patch Set 9 : Fix js compile error in unused path #Patch Set 10 : Fixup overlooked test revealed by CQ. #
Messages
Total messages: 46 (24 generated)
Description was changed from ========== Update CUPS settings UI to allow USB printers to be added via discovery. This re-enables the "discovery" dialog when a user asks to add a printer; the dialog is populated with any USB printers that could not be set up automatically. (Eventually, this will also be populated with auto-discovered network printers). If the user selects a printer from the list, they are taken to the manufacturer-model dialog to select a driver. This change also alters a button in the ui. Right now, there's always an "Add printer manually" button in the manufacturer-model dialog. But this *actually* functions as a "back" button when adding a printer manually, as it takes you back to the previous dialog with the values you entered there, and, in the discovery case, this button doesn't make sense as it doesn't have previous values to take you back to. So the button has been re-texted with 'back' in the manual case, and hidden completely in the discovery case. Finally, this change also fixes a few things in URI handling for usb printers in cups_printers_handler so that, when we add a printer from the manufacturer-model dialog, we get a correct URI to do so. BUG=616866 ========== to ========== Update CUPS settings UI to allow USB printers to be added via discovery. This re-enables the "discovery" dialog when a user asks to add a printer; the dialog is populated with any USB printers that could not be set up automatically. (Eventually, this will also be populated with auto-discovered network printers). If the user selects a printer from the list, they are taken to the manufacturer-model dialog to select a driver. This change also alters a button in the ui. Right now, there's always an "Add printer manually" button in the manufacturer-model dialog. But this *actually* functions as a "back" button when adding a printer manually, as it takes you back to the previous dialog with the values you entered there, and, in the discovery case, this button doesn't make sense as it doesn't have previous values to take you back to. So the button has been re-texted with 'back' in the manual case, and hidden completely in the discovery case. Finally, this change also fixes a few things in URI handling for usb printers in cups_printers_handler so that, when we add a printer from the manufacturer-model dialog, we get a correct URI to do so. BUG=616866 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
justincarlson@chromium.org changed reviewers: + xdai@chromium.org
Daisy, can you take an initial look at this and tell me how messed up this is? Apologies in advance for all the things I'm sure I'm doing wrong on the UI front. :)
On 2017/04/18 21:05:02, Carlson wrote: > Daisy, can you take an initial look at this and tell me how messed up this is? > Apologies in advance for all the things I'm sure I'm doing wrong on the UI > front. :) Friendly ping?
On 2017/04/24 19:01:33, Carlson wrote: > On 2017/04/18 21:05:02, Carlson wrote: > > Daisy, can you take an initial look at this and tell me how messed up this is? > > > Apologies in advance for all the things I'm sure I'm doing wrong on the UI > > front. :) > > Friendly ping? I'm so sorry I totally forgot this. I was gardener last week and was in a big mess. Will review it after lunch.
Don't stress about getting it done today if that's not convenient, just wanted to make sure it wasn't lost. If I can get an initial look in the next couple of days that would be great. -J On Mon, Apr 24, 2017 at 12:15 PM <xdai@chromium.org> wrote: > On 2017/04/24 19:01:33, Carlson wrote: > > On 2017/04/18 21:05:02, Carlson wrote: > > > Daisy, can you take an initial look at this and tell me how messed up > this > is? > > > > > Apologies in advance for all the things I'm sure I'm doing wrong on > the UI > > > front. :) > > > > Friendly ping? > > I'm so sorry I totally forgot this. I was gardener last week and was in a > big > mess. Will review it after lunch. > > https://codereview.chromium.org/2825153002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2825153002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2825153002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:56: notify: true, I think put the new property newPrinter in manufacturer-model-dialog might make more sense. Previously the flow is discovery-dialog->configuring-dialog and manual-add-printer-dialog->manufacturer-model-dialog->configuring-dialog. Now the flow changed. So I think some of the codes might need to be revisited to make sure they still make sense. For example, openConfiguringPrinterDialog_() may not work as expected after your change. https://codereview.chromium.org/2825153002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:357: a manual flow (as opposed to a discovery flow). */ The format should be /** * @type {boolean} comment... * (comment to be continued...) */ https://codereview.chromium.org/2825153002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:358: inManualFlow: { this should be a private property.
Some responses, some questions. PTAL? https://codereview.chromium.org/2825153002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2825153002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:56: notify: true, On 2017/04/24 21:42:12, xdai1 wrote: > I think put the new property newPrinter in manufacturer-model-dialog might make > more sense. > > Previously the flow is discovery-dialog->configuring-dialog and > manual-add-printer-dialog->manufacturer-model-dialog->configuring-dialog. Now > the flow changed. So I think some of the codes might need to be revisited to > make sure they still make sense. For example, openConfiguringPrinterDialog_() > may not work as expected after your change. Sorry, I don't really understand what you mean by putting newPrinter in manufacturer-model-dialog. I think this really means I don't fully understand how Polymer data binding works. So I'm going to describe what I'm trying to do, and hopefully you can point me at the right way to do it. :) When I have the discovery dialog open, and the user selects a printer, I need to get the information about the selected printer over to the manufacturer-model dialog, because the user needs to supply more information before we can set up the discovered printer. (Eventually we'll be in a situation where we only want to go to the manu/model dialog if the printer setup fails. However, right now we only discover USB devices that are not configured, and since USB printer configuration is attempted automatically when the device is plugged in, the only way a USB printer shows up in the discovery list is if configuration failed and we need more information to set it up). So what I want to have happen is that add-printer-manufacturer-model's 'newPrinter' property is set to whatever the discovery-dialog's 'selectedPrinter' property was when we change dialogs. The way (I understand) this works in this code is that we bind both add-printer-discovery-dialog.newPrinter and add-printer-manufacturer-model-dialog.newPrinter to settings-cups-add-printer-dialog.newPrinter which makes them (effectively) the same object. (I gather there's some serialization and deserialization going on behind the scenes, but I think that's not important here?) So given how this works, I think I need the newPrinter property declared in all three places -- the top level dialog and both the discovery and manufacturer-model dialogs. (note that the previous version of this that I sent to you effectively bound add-printer-discovery-dialog.selectedPrinter to newPrinter, but you correctly pointed out that we only want to do this if the user has actually decided to add a discovered printer, otherwise we mess up the state of newPrinter). So what am I missing here? https://codereview.chromium.org/2825153002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:357: a manual flow (as opposed to a discovery flow). */ On 2017/04/24 21:42:12, xdai1 wrote: > The format should be > /** > * @type {boolean} comment... > * (comment to be continued...) > */ done. https://codereview.chromium.org/2825153002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:358: inManualFlow: { On 2017/04/24 21:42:12, xdai1 wrote: > this should be a private property. Done.
Please see my inline comment. Btw: I didn't see the new patch. Did you forget to upload it? https://codereview.chromium.org/2825153002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2825153002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:56: notify: true, On 2017/05/01 20:59:49, Carlson wrote: > On 2017/04/24 21:42:12, xdai1 wrote: > > I think put the new property newPrinter in manufacturer-model-dialog might > make > > more sense. > > > > Previously the flow is discovery-dialog->configuring-dialog and > > manual-add-printer-dialog->manufacturer-model-dialog->configuring-dialog. Now > > the flow changed. So I think some of the codes might need to be revisited to > > make sure they still make sense. For example, openConfiguringPrinterDialog_() > > may not work as expected after your change. > > Sorry, I don't really understand what you mean by putting newPrinter in > manufacturer-model-dialog. > > I think this really means I don't fully understand how Polymer data binding > works. So I'm going to describe what I'm trying to do, and hopefully you can > point me at the right way to do it. :) > > When I have the discovery dialog open, and the user selects a printer, I need to > get the information about the selected printer over to the manufacturer-model > dialog, because the user needs to supply more information before we can set up > the discovered printer. (Eventually we'll be in a situation where we only want > to go to the manu/model dialog if the printer setup fails. However, right now > we only discover USB devices that are not configured, and since USB printer > configuration is attempted automatically when the device is plugged in, the only > way a USB printer shows up in the discovery list is if configuration failed and > we need more information to set it up). > > So what I want to have happen is that add-printer-manufacturer-model's > 'newPrinter' property is set to whatever the discovery-dialog's > 'selectedPrinter' property was when we change dialogs. > > The way (I understand) this works in this code is that we bind both > add-printer-discovery-dialog.newPrinter and > add-printer-manufacturer-model-dialog.newPrinter to > settings-cups-add-printer-dialog.newPrinter which makes them (effectively) the > same object. (I gather there's some serialization and deserialization going on > behind the scenes, but I think that's not important here?) > > So given how this works, I think I need the newPrinter property declared in all > three places -- the top level dialog and both the discovery and > manufacturer-model dialogs. > > (note that the previous version of this that I sent to you effectively bound > add-printer-discovery-dialog.selectedPrinter to newPrinter, but you correctly > pointed out that we only want to do this if the user has actually decided to add > a discovered printer, otherwise we mess up the state of newPrinter). > > So what am I missing here? Sorry I didn't explain it in a clear way. I understand what you were trying to do here and functionally it can do what you want: data-binding works as expected as you described. However, IMO I think it might be better to do it in another way. Previously when I implemented this, the expected flow is when a user selected a printer from discovery-dialog, it would jump directly to configuring-dialog. manufacturer-model-dialog was only used in manual-add-printer flow. So manufacturer-model-dialog only needs maintain the 'newPrinter' property from manual-add-printer-dialog. Now we changed the flow: when a user selectes a printer from discovery-dialog, it jumps to manufacturer-model-dialog and then to configuring-dialog. I think instead of putting 'newPrinter' property in discovery-dialog, we should put 'selectedPrinter' property in manufacturer-model-dialog. This way, discovery-dialog only maintains 'selectedPrinter' property (and that's the only thing it should know), and manufacturer-model-dialog maintains 'newPrinter' property from manual-add-printer-dialog and 'selectedPrinter' property from discovery-dialog, and the two properties are binded to the corresponding properties in settings-cups-add-printer-dialog. I think this way we don't mess up the two flows: auto-discovery flow and manual-setup-flow and also keep two properties for the two flows: selectedPrinter for auto-discovery-flow and newPrinter for manual-setup-flow. What do you think?
On 2017/05/02 17:25:08, xdai1 wrote: > Please see my inline comment. > > Btw: I didn't see the new patch. Did you forget to upload it? > > https://codereview.chromium.org/2825153002/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js > (right): > > https://codereview.chromium.org/2825153002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:56: > notify: true, > On 2017/05/01 20:59:49, Carlson wrote: > > On 2017/04/24 21:42:12, xdai1 wrote: > > > I think put the new property newPrinter in manufacturer-model-dialog might > > make > > > more sense. > > > > > > Previously the flow is discovery-dialog->configuring-dialog and > > > manual-add-printer-dialog->manufacturer-model-dialog->configuring-dialog. > Now > > > the flow changed. So I think some of the codes might need to be revisited to > > > make sure they still make sense. For example, > openConfiguringPrinterDialog_() > > > may not work as expected after your change. > > > > Sorry, I don't really understand what you mean by putting newPrinter in > > manufacturer-model-dialog. > > > > I think this really means I don't fully understand how Polymer data binding > > works. So I'm going to describe what I'm trying to do, and hopefully you can > > point me at the right way to do it. :) > > > > When I have the discovery dialog open, and the user selects a printer, I need > to > > get the information about the selected printer over to the manufacturer-model > > dialog, because the user needs to supply more information before we can set up > > the discovered printer. (Eventually we'll be in a situation where we only > want > > to go to the manu/model dialog if the printer setup fails. However, right now > > we only discover USB devices that are not configured, and since USB printer > > configuration is attempted automatically when the device is plugged in, the > only > > way a USB printer shows up in the discovery list is if configuration failed > and > > we need more information to set it up). > > > > So what I want to have happen is that add-printer-manufacturer-model's > > 'newPrinter' property is set to whatever the discovery-dialog's > > 'selectedPrinter' property was when we change dialogs. > > > > The way (I understand) this works in this code is that we bind both > > add-printer-discovery-dialog.newPrinter and > > add-printer-manufacturer-model-dialog.newPrinter to > > settings-cups-add-printer-dialog.newPrinter which makes them (effectively) the > > same object. (I gather there's some serialization and deserialization going > on > > behind the scenes, but I think that's not important here?) > > > > So given how this works, I think I need the newPrinter property declared in > all > > three places -- the top level dialog and both the discovery and > > manufacturer-model dialogs. > > > > (note that the previous version of this that I sent to you effectively bound > > add-printer-discovery-dialog.selectedPrinter to newPrinter, but you correctly > > pointed out that we only want to do this if the user has actually decided to > add > > a discovered printer, otherwise we mess up the state of newPrinter). > > > > So what am I missing here? > > Sorry I didn't explain it in a clear way. > > I understand what you were trying to do here and functionally it can do what you > want: data-binding works as expected as you described. However, IMO I think it > might be better to do it in another way. > > Previously when I implemented this, the expected flow is when a user selected a > printer from discovery-dialog, it would jump directly to configuring-dialog. > manufacturer-model-dialog was only used in manual-add-printer flow. So > manufacturer-model-dialog only needs maintain the 'newPrinter' property from > manual-add-printer-dialog. > > Now we changed the flow: when a user selectes a printer from discovery-dialog, > it jumps to manufacturer-model-dialog and then to configuring-dialog. I think > instead of putting 'newPrinter' property in discovery-dialog, we should put > 'selectedPrinter' property in manufacturer-model-dialog. This way, > discovery-dialog only maintains 'selectedPrinter' property (and that's the only > thing it should know), and manufacturer-model-dialog maintains 'newPrinter' > property from manual-add-printer-dialog and 'selectedPrinter' property from > discovery-dialog, and the two properties are binded to the corresponding > properties in settings-cups-add-printer-dialog. > > I think this way we don't mess up the two flows: auto-discovery flow and > manual-setup-flow and also keep two properties for the two flows: > selectedPrinter for auto-discovery-flow and newPrinter for manual-setup-flow. > What do you think? Given that the manufacturer and model selections are directly bound to newPrinter.printerManufacturer and newPrinter.printerModel, it's a little hard for me to put selectedPrinter into manufacturer-model dialog, but I did remove newPrinter from discovery dialog, and moved the "copy fields from selectedPrinter to newPrinter into the top-level dialog, invoking it based on the previous dialog when switching dialog. Does this look reasonable? Or am I still not quite understanding what you want? Thanks! -J
Hey Carlson, I found it's a little hard for me to explain it clearly in words. So I wrote a draft CL to illustrate my thought: https://codereview.chromium.org/2876563003/. I didn't run the CL locally so there might be bugs in it. But the idea should be straight. Let me know if you think it's better or not. https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html (right): https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:320: on-tap="switchToManualAddDialog_" hidden="[[!inManualFlow]]"> there is no inManualFlow property in add-printer-manufacturer-model-dialog? https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:346: </style> indent off https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:370: new-printer="{{newPrinter}}"> I still feel like add-printer-discovery-dialog should not have a newPrinter property. https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:392: in-manual-flow="[[inManualFlow]]"> Does this work? If add-printer-manufacturer-model-dialog doesn't have inManualFlow, you may not be able use in-manual-flow="[[inManualFlow]]. https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:469: this.newPrinter = Object.assign({}, this.selectedPrinter); The reason I prefer not "copying fields over" either in top-level dialog or in low-level dialog is the printer data from two flows (auto-discovery and manual) are not mixed up.
> So I wrote a draft CL to illustrate my thought: > https://codereview.chromium.org/2876563003/. I didn't run the CL locally so > there might be bugs in it. But the idea should be straight. Let me know if you > think it's better or not. I appreciate you taking the time to do this, I think I understand what you're going for now. I think the advantage of what you're doing is that you ensure the contents of 'newPrinter' remain unmodified when we go through the discovery->manufacturer-model dialog flow. So if the user does something like 1. Click "add a printers" 2. Click "Add Manually", fill in some information 3. *Cancel* that add, go back to the discovery dialog and select a printer 4. Cancel that and return to the "Add Manually" dialog then the information they initially entered will be preserved with your approach, whereas it will be blown away by my approach. However, I don't think this flow is possible? There's no way to go from the discovery flow back to the manual flow without restarting the entire dialog. Or if there is a way to do that, I don't see it. So I think it's better to minimize the amount of "if (inManualFlow)" stuff, as that's going to be a somewhat subtle thing for people to get right in the future; I think it's better to have people think "just update newPrinter if you're in manufacturer-model" instead of "update selectedPrinter or newPrinter depending on where you came from". Does that sound reasonable? You are a lot more knowledgeable about how ui should work than me, so if you want me to convert this to keep the two fields separate, I will happily do that. https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html (right): https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:320: on-tap="switchToManualAddDialog_" hidden="[[!inManualFlow]]"> On 2017/05/10 20:12:53, xdai1 wrote: > there is no inManualFlow property in add-printer-manufacturer-model-dialog? This is interesting, because I see what you're saying, yet this does seem to work. The back button is hidden if I get there from discovery, and not if I get there from add manually. How does scoping work here? Maybe it's legitimate to reference inManualFlow from the enclosing settings-cups-add-printer-dialog scope whence this is instantiated? If that's true, we probably don't need some of the newPrinter and selectedPrinter bindings as well. https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:346: </style> On 2017/05/10 20:12:53, xdai1 wrote: > indent off Done. https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:370: new-printer="{{newPrinter}}"> On 2017/05/10 20:12:53, xdai1 wrote: > I still feel like add-printer-discovery-dialog should not have a newPrinter > property. You're right, this isn't needed anymore, I just forgot to remove this. https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:392: in-manual-flow="[[inManualFlow]]"> On 2017/05/10 20:12:53, xdai1 wrote: > Does this work? If add-printer-manufacturer-model-dialog doesn't have > inManualFlow, you may not be able use in-manual-flow="[[inManualFlow]]. It does seem to work. Do you want me to add the property explicitly anyways? Not sure if we have redundant code elsewhere, or what I'm doing here is implicitly bad style. https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:469: this.newPrinter = Object.assign({}, this.selectedPrinter); On 2017/05/10 20:12:53, xdai1 wrote: > The reason I prefer not "copying fields over" either in top-level dialog or in > low-level dialog is the printer data from two flows (auto-discovery and manual) > are not mixed up. I think I understand this now. Replied at top level
On 2017/05/10 21:28:18, Carlson wrote: > I think the advantage of what you're doing is that you ensure the contents of > 'newPrinter' remain unmodified when we go through the > discovery->manufacturer-model dialog flow. So if the user does something like > > 1. Click "add a printers" > 2. Click "Add Manually", fill in some information > 3. *Cancel* that add, go back to the discovery dialog and select a printer > 4. Cancel that and return to the "Add Manually" dialog > > then the information they initially entered will be preserved with your > approach, whereas it will be blown away by my approach. > > However, I don't think this flow is possible? There's no way to go from the > discovery flow back to the manual flow without restarting the entire dialog. Or > if there is a way to do that, I don't see it. So I think it's better to > minimize the amount of "if (inManualFlow)" stuff, as that's going to be a > somewhat subtle thing for people to get right in the future; I think it's better > to have people think "just update newPrinter if you're in manufacturer-model" > instead of "update selectedPrinter or newPrinter depending on where you came > from". > > Does that sound reasonable? You are a lot more knowledgeable about how ui > should work than me, so if you want me to convert this to keep the two fields > separate, I will happily do that. > Actually CUPS printing is my first Polymer project so I'm only a few months more knowledgeable than you. Most of the time it looks like a magic black box for me as well:) Yes I agree that the flow you described here is impossible if we only have a back button in manual flow. But if we also have a back button in discovery flow, or if we have a button to jump from manual flow to discovery flow (like before), then things might change. I have no strong objection to your implementation here. But if you decide to go with this approach, please rename "newPrinter" to "printerInSetup" in "add-printer-manufacturer-model-dialog" and "settings-cups-add-printer-dialog" to make it distinguishable from "selectedPrinter" and "newPrinter". And also please make all the other places right that have changed because of your change. For example, "selectedPrinter" in "settings-cups-add-printer-dialog" is no longer used. https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html (right): https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:320: on-tap="switchToManualAddDialog_" hidden="[[!inManualFlow]]"> On 2017/05/10 21:28:17, Carlson wrote: > On 2017/05/10 20:12:53, xdai1 wrote: > > there is no inManualFlow property in add-printer-manufacturer-model-dialog? > > This is interesting, because I see what you're saying, yet this does seem to > work. The back button is hidden if I get there from discovery, and not if I get > there from add manually. > > How does scoping work here? Maybe it's legitimate to reference inManualFlow > from the enclosing settings-cups-add-printer-dialog scope whence this is > instantiated? If that's true, we probably don't need some of the newPrinter and > selectedPrinter bindings as well. Not sure exactly why it works here. But I think when "hidden" is binded to "inManualFlow", Polymer will generate an implicit property "inManualFlow" that can also be used in "settings-cups-add-printer-dialog" binding. However, I think we may not be able get rid of newPrinter and selectedPrinter bindings here. It may be fine if you don't use these implicit properties in the js file. But if you do need to use the property in the js file, you need to define the property explicitly. https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:392: in-manual-flow="[[inManualFlow]]"> On 2017/05/10 21:28:17, Carlson wrote: > On 2017/05/10 20:12:53, xdai1 wrote: > > Does this work? If add-printer-manufacturer-model-dialog doesn't have > > inManualFlow, you may not be able use in-manual-flow="[[inManualFlow]]. > > It does seem to work. Do you want me to add the property explicitly anyways? > Not sure if we have redundant code elsewhere, or what I'm doing here is > implicitly bad style. See my guess in the above comment. If my guess is correct, then we can leave the code as is since we don't use the property in the js file anyway. Otherwise, it might be better to add it back explicitly. https://codereview.chromium.org/2825153002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html (right): https://codereview.chromium.org/2825153002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:320: on-tap="switchToManualAddDialog_" hidden="[[!inManualFlow]]"> Unrelated question: If we want to modify this button to be a back button, why don't we have it for discovery flow as well? Clicking on the back button either directs the user to the manual dialog or to the discovery dialog seems more reasonable? (btw: was weifangsun@ aware of this button change?)
Description was changed from ========== Update CUPS settings UI to allow USB printers to be added via discovery. This re-enables the "discovery" dialog when a user asks to add a printer; the dialog is populated with any USB printers that could not be set up automatically. (Eventually, this will also be populated with auto-discovered network printers). If the user selects a printer from the list, they are taken to the manufacturer-model dialog to select a driver. This change also alters a button in the ui. Right now, there's always an "Add printer manually" button in the manufacturer-model dialog. But this *actually* functions as a "back" button when adding a printer manually, as it takes you back to the previous dialog with the values you entered there, and, in the discovery case, this button doesn't make sense as it doesn't have previous values to take you back to. So the button has been re-texted with 'back' in the manual case, and hidden completely in the discovery case. Finally, this change also fixes a few things in URI handling for usb printers in cups_printers_handler so that, when we add a printer from the manufacturer-model dialog, we get a correct URI to do so. BUG=616866 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Update CUPS settings UI to allow USB printers to be added via discovery. This re-enables the "discovery" dialog when a user asks to add a printer; the dialog is populated with any USB printers that could not be set up automatically. (Eventually, this will also be populated with auto-discovered network printers). If the user selects a printer from the list, they are taken to the manufacturer-model dialog to select a driver. This change also alters a button in the ui. Right now, there's always an "Add printer manually" button in the manufacturer-model dialog. But this *actually* functions as a "back" button when adding a printer manually, as it takes you back to the previous dialog with the values you entered there, and, in the discovery case, this button doesn't make sense as it doesn't have previous values to take you back to. So the button has been re-texted with 'back', and now takes you to the previous dialog, whether that was discovery or manual addition. Finally, this change also fixes a few things in URI handling for usb printers in cups_printers_handler so that, when we add a printer from the manufacturer-model dialog, we get a correct URI to do so. BUG=616866 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Appreciate your patience and help on this. PTAL? https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html (right): https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:320: on-tap="switchToManualAddDialog_" hidden="[[!inManualFlow]]"> On 2017/05/11 00:25:10, xdai1 wrote: > On 2017/05/10 21:28:17, Carlson wrote: > > On 2017/05/10 20:12:53, xdai1 wrote: > > > there is no inManualFlow property in add-printer-manufacturer-model-dialog? > > > > This is interesting, because I see what you're saying, yet this does seem to > > work. The back button is hidden if I get there from discovery, and not if I > get > > there from add manually. > > > > How does scoping work here? Maybe it's legitimate to reference inManualFlow > > from the enclosing settings-cups-add-printer-dialog scope whence this is > > instantiated? If that's true, we probably don't need some of the newPrinter > and > > selectedPrinter bindings as well. > > Not sure exactly why it works here. But I think when "hidden" is binded to > "inManualFlow", Polymer will generate an implicit property "inManualFlow" that > can also be used in "settings-cups-add-printer-dialog" binding. However, I think > we may not be able get rid of newPrinter and selectedPrinter bindings here. It > may be fine if you don't use these implicit properties in the js file. But if > you do need to use the property in the js file, you need to define the property > explicitly. Should be better now. https://codereview.chromium.org/2825153002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:392: in-manual-flow="[[inManualFlow]]"> On 2017/05/11 00:25:10, xdai1 wrote: > On 2017/05/10 21:28:17, Carlson wrote: > > On 2017/05/10 20:12:53, xdai1 wrote: > > > Does this work? If add-printer-manufacturer-model-dialog doesn't have > > > inManualFlow, you may not be able use in-manual-flow="[[inManualFlow]]. > > > > It does seem to work. Do you want me to add the property explicitly anyways? > > Not sure if we have redundant code elsewhere, or what I'm doing here is > > implicitly bad style. > > See my guess in the above comment. If my guess is correct, then we can leave the > code as is since we don't use the property in the js file anyway. Otherwise, it > might be better to add it back explicitly. I think this is better now. inManualFlow no longer exists, but we do propagate previousDialog down, and explicitly declare it as a property in this dialog. https://codereview.chromium.org/2825153002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html (right): https://codereview.chromium.org/2825153002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:320: on-tap="switchToManualAddDialog_" hidden="[[!inManualFlow]]"> On 2017/05/11 00:25:10, xdai1 wrote: > Unrelated question: If we want to modify this button to be a back button, why > don't we have it for discovery flow as well? Clicking on the back button either > directs the user to the manual dialog or to the discovery dialog seems more > reasonable? (btw: was weifangsun@ aware of this button change?) Good point on both parts. We discussed options offline. I've pinged, but haven't heard back from Weifang, but I'd really like to get the top level change in for M60, so for now I've implemented this as 'back' in both the discovery and manual case, which is sort of the minimal change that makes things orthogonal. I recognize that we may change this ui decision later, but I think it's not worth holding up the change?
Please see my inline comments. Thanks for bearing with me! https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:142: this.selectedPrinter.printerPPDPath = ''; You probably don't need these three lines. After the button changes to a back button, currently there is no way to jump from manually-add-printer dialog to auto-discovery dialog so the data from manually-add-printer flow won't affect the auto-discovery flow. https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:156: value: getEmptyPrinter_ getEmptyPrinter_()? https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:420: this.switchDialog_( After your change, the previous dialog for configuring-printer-dialog only can be manufacturer-model-dialog. So this function won't do the right thing any more. https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:444: if (this.previousDialog == AddPrinterDialogs.MANUALLY) { Same as above. https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:478: getConfiguringPrinterName_: function() { This function can be removed now. And in configuring-printer-dialog, bind printer-name="[[newPrinter.printerName]]"
PTAL? https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:142: this.selectedPrinter.printerPPDPath = ''; On 2017/05/11 22:50:28, xdai1 wrote: > You probably don't need these three lines. After the button changes to a back > button, currently there is no way to jump from manually-add-printer dialog to > auto-discovery dialog so the data from manually-add-printer flow won't affect > the auto-discovery flow. That's true, but when we actually do select a printer, we may end up with partial information that we don't want to use. This actually happens in the USB case, so these lines are needed. https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:156: value: getEmptyPrinter_ On 2017/05/11 22:50:28, xdai1 wrote: > getEmptyPrinter_()? I don't think so? https://www.polymer-project.org/1.0/docs/devguide/properties says value needs to be a function that returns the proper type if the default value should be an Array or Object, so I think we need to pass the function itself, not the result of the function. https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:420: this.switchDialog_( On 2017/05/11 22:50:28, xdai1 wrote: > After your change, the previous dialog for configuring-printer-dialog only can > be manufacturer-model-dialog. So this function won't do the right thing any > more. That's true right now, but isn't the right thing in the long term. When network discovered printers go in, we will want to try to configure them directly from the discovery screen, we just don't do that for USB printers. So I think leaving this as-is is the right thing? I don't think it's wrong, I think we just never hit the first conditional right now. https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:444: if (this.previousDialog == AddPrinterDialogs.MANUALLY) { On 2017/05/11 22:50:28, xdai1 wrote: > Same as above. Same reply here. Eventually this will be needed and I think it's not incorrect for now. https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:478: getConfiguringPrinterName_: function() { On 2017/05/11 22:50:28, xdai1 wrote: > This function can be removed now. And in configuring-printer-dialog, bind > printer-name="[[newPrinter.printerName]]" Good point, done.
justincarlson@chromium.org changed reviewers: + stevenjb@chromium.org
+stevenjb because I think xdai is almost satisfied and I'm hoping to catch M60 here.
Almost! https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:142: this.selectedPrinter.printerPPDPath = ''; On 2017/05/11 23:57:10, Carlson wrote: > On 2017/05/11 22:50:28, xdai1 wrote: > > You probably don't need these three lines. After the button changes to a back > > button, currently there is no way to jump from manually-add-printer dialog to > > auto-discovery dialog so the data from manually-add-printer flow won't affect > > the auto-discovery flow. > > That's true, but when we actually do select a printer, we may end up with > partial information that we don't want to use. This actually happens in the USB > case, so these lines are needed. Acknowledged. https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:156: value: getEmptyPrinter_ On 2017/05/11 23:57:11, Carlson wrote: > On 2017/05/11 22:50:28, xdai1 wrote: > > getEmptyPrinter_()? > > I don't think so? > > https://www.polymer-project.org/1.0/docs/devguide/properties > > says value needs to be a function that returns the proper type if the default > value should be an Array or Object, so I think we need to pass the function > itself, not the result of the function. Acknowledged. https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:420: this.switchDialog_( On 2017/05/11 23:57:11, Carlson wrote: > On 2017/05/11 22:50:28, xdai1 wrote: > > After your change, the previous dialog for configuring-printer-dialog only can > > be manufacturer-model-dialog. So this function won't do the right thing any > > more. > > That's true right now, but isn't the right thing in the long term. When network > discovered printers go in, we will want to try to configure them directly from > the discovery screen, we just don't do that for USB printers. > > So I think leaving this as-is is the right thing? I don't think it's wrong, I > think we just never hit the first conditional right now. > Because the previous dialog will always be this.previousDialog == AddPrinterDialogs.MANUFACTURER, we set |this.configuringDialogTitle| to be the manuall-add-printer title, which is incorrect. https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:444: if (this.previousDialog == AddPrinterDialogs.MANUALLY) { On 2017/05/11 23:57:10, Carlson wrote: > On 2017/05/11 22:50:28, xdai1 wrote: > > Same as above. > > Same reply here. Eventually this will be needed and I think it's not incorrect > for now. Acknowledged.
https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:420: this.switchDialog_( On 2017/05/12 00:30:43, xdai1 wrote: > On 2017/05/11 23:57:11, Carlson wrote: > > On 2017/05/11 22:50:28, xdai1 wrote: > > > After your change, the previous dialog for configuring-printer-dialog only > can > > > be manufacturer-model-dialog. So this function won't do the right thing any > > > more. > > > > That's true right now, but isn't the right thing in the long term. When > network > > discovered printers go in, we will want to try to configure them directly from > > the discovery screen, we just don't do that for USB printers. > > > > So I think leaving this as-is is the right thing? I don't think it's wrong, I > > think we just never hit the first conditional right now. > > > > Because the previous dialog will always be this.previousDialog == > AddPrinterDialogs.MANUFACTURER, we set |this.configuringDialogTitle| to be the > manuall-add-printer title, which is incorrect. Took me a while, but I understand now. I'm not sure what the right thing is here, it's really a question of what's least confusing to the user and what it means to add a printer 'manually'. We can sort of think of this as having 3 categories: 1) Printers that magically work. You select them in the discovery box, click add, no further action is required. (Or, like USB, they just show up as available) 2) Printers that aren't discovered at all. The user puts in name, address, protocol, manufacturer, model. 3) Printers that are discovered, but require the user to tell us manufacturer/model before they can be configured. 2 is definitely manual. 1 is definitely *not* manual. 3 is less clear. I've been calling 3 'manual' as well, which is why I was confused; I think you tend to think of those as *not* 'manual'. I think under my definition this is correct and under yours it's not, but neither one is obviously right. Also note that, as currently written, the manufacturer-model dialog always says "Add a printer manually", even when we get there from discovery, which I've also thought was the right thing. You OK with me punting on this point by filing a bug to clarify dialog titles? I'd like to get Weifang's input on it. https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:444: if (this.previousDialog == AddPrinterDialogs.MANUALLY) { On 2017/05/12 00:30:43, xdai1 wrote: > On 2017/05/11 23:57:10, Carlson wrote: > > On 2017/05/11 22:50:28, xdai1 wrote: > > > Same as above. > > > > Same reply here. Eventually this will be needed and I think it's not > incorrect > > for now. > > Acknowledged. Actually, this one is a real problem looking more closely. Eventually we'll be getting to "configuring" from any of {DISCOVERY, MANUALLY, MANUFACTURER}, so we should go back to the right place from any of those. I've done that. (As an aside, I think I'm going to do a future change that alters how we manage previousDialog and the domIfBooleanName stuff; I think there's a way to make that a little more elegant)
owner lgtm, but please wait for lg from xdai@ https://codereview.chromium.org/2825153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2825153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:217: } nit: no {}
lgtm. Thanks for doing this! https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:420: this.switchDialog_( On 2017/05/12 17:12:46, Carlson wrote: > On 2017/05/12 00:30:43, xdai1 wrote: > > On 2017/05/11 23:57:11, Carlson wrote: > > > On 2017/05/11 22:50:28, xdai1 wrote: > > > > After your change, the previous dialog for configuring-printer-dialog only > > can > > > > be manufacturer-model-dialog. So this function won't do the right thing > any > > > > more. > > > > > > That's true right now, but isn't the right thing in the long term. When > > network > > > discovered printers go in, we will want to try to configure them directly > from > > > the discovery screen, we just don't do that for USB printers. > > > > > > So I think leaving this as-is is the right thing? I don't think it's wrong, > I > > > think we just never hit the first conditional right now. > > > > > > > Because the previous dialog will always be this.previousDialog == > > AddPrinterDialogs.MANUFACTURER, we set |this.configuringDialogTitle| to be the > > manuall-add-printer title, which is incorrect. > > Took me a while, but I understand now. > > I'm not sure what the right thing is here, it's really a question of what's > least confusing to the user and what it means to add a printer 'manually'. > > We can sort of think of this as having 3 categories: > > 1) Printers that magically work. You select them in the discovery box, click > add, no further action is required. (Or, like USB, they just show up as > available) > > 2) Printers that aren't discovered at all. The user puts in name, address, > protocol, manufacturer, model. > > 3) Printers that are discovered, but require the user to tell us > manufacturer/model before they can be configured. > > 2 is definitely manual. 1 is definitely *not* manual. > > 3 is less clear. I've been calling 3 'manual' as well, which is why I was > confused; I think you tend to think of those as *not* 'manual'. I think under > my definition this is correct and under yours it's not, but neither one is > obviously right. > > Also note that, as currently written, the manufacturer-model dialog always says > "Add a printer manually", even when we get there from discovery, which I've also > thought was the right thing. > > You OK with me punting on this point by filing a bug to clarify dialog titles? > I'd like to get Weifang's input on it. Yes. I'm fine to file a bug for this.
Thanks all! https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2825153002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:420: this.switchDialog_( On 2017/05/12 17:56:55, xdai1 wrote: > On 2017/05/12 17:12:46, Carlson wrote: > > On 2017/05/12 00:30:43, xdai1 wrote: > > > On 2017/05/11 23:57:11, Carlson wrote: > > > > On 2017/05/11 22:50:28, xdai1 wrote: > > > > > After your change, the previous dialog for configuring-printer-dialog > only > > > can > > > > > be manufacturer-model-dialog. So this function won't do the right thing > > any > > > > > more. > > > > > > > > That's true right now, but isn't the right thing in the long term. When > > > network > > > > discovered printers go in, we will want to try to configure them directly > > from > > > > the discovery screen, we just don't do that for USB printers. > > > > > > > > So I think leaving this as-is is the right thing? I don't think it's > wrong, > > I > > > > think we just never hit the first conditional right now. > > > > > > > > > > Because the previous dialog will always be this.previousDialog == > > > AddPrinterDialogs.MANUFACTURER, we set |this.configuringDialogTitle| to be > the > > > manuall-add-printer title, which is incorrect. > > > > Took me a while, but I understand now. > > > > I'm not sure what the right thing is here, it's really a question of what's > > least confusing to the user and what it means to add a printer 'manually'. > > > > We can sort of think of this as having 3 categories: > > > > 1) Printers that magically work. You select them in the discovery box, click > > add, no further action is required. (Or, like USB, they just show up as > > available) > > > > 2) Printers that aren't discovered at all. The user puts in name, address, > > protocol, manufacturer, model. > > > > 3) Printers that are discovered, but require the user to tell us > > manufacturer/model before they can be configured. > > > > 2 is definitely manual. 1 is definitely *not* manual. > > > > 3 is less clear. I've been calling 3 'manual' as well, which is why I was > > confused; I think you tend to think of those as *not* 'manual'. I think under > > my definition this is correct and under yours it's not, but neither one is > > obviously right. > > > > Also note that, as currently written, the manufacturer-model dialog always > says > > "Add a printer manually", even when we get there from discovery, which I've > also > > thought was the right thing. > > > > You OK with me punting on this point by filing a bug to clarify dialog titles? > > > I'd like to get Weifang's input on it. > > Yes. I'm fine to file a bug for this. Filed crbug/721841 https://codereview.chromium.org/2825153002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2825153002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:217: } On 2017/05/12 17:19:09, stevenjb wrote: > nit: no {} As I understand it, it's optional in C++ and doing braces here is consistent with the rest of the file, so I'd like to leave this.
The CQ bit was checked by justincarlson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by justincarlson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by justincarlson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by justincarlson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by justincarlson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, xdai@chromium.org Link to the patchset: https://codereview.chromium.org/2825153002/#ps180001 (title: "Fixup overlooked test revealed by CQ.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494874054453780,
"parent_rev": "1dd94e01da63c05f3c6855ac25a911f2d43868ae", "commit_rev":
"2ed79b998d112e09fc22678cd4bd838a465ee569"}
Message was sent while issue was closed.
Description was changed from ========== Update CUPS settings UI to allow USB printers to be added via discovery. This re-enables the "discovery" dialog when a user asks to add a printer; the dialog is populated with any USB printers that could not be set up automatically. (Eventually, this will also be populated with auto-discovered network printers). If the user selects a printer from the list, they are taken to the manufacturer-model dialog to select a driver. This change also alters a button in the ui. Right now, there's always an "Add printer manually" button in the manufacturer-model dialog. But this *actually* functions as a "back" button when adding a printer manually, as it takes you back to the previous dialog with the values you entered there, and, in the discovery case, this button doesn't make sense as it doesn't have previous values to take you back to. So the button has been re-texted with 'back', and now takes you to the previous dialog, whether that was discovery or manual addition. Finally, this change also fixes a few things in URI handling for usb printers in cups_printers_handler so that, when we add a printer from the manufacturer-model dialog, we get a correct URI to do so. BUG=616866 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Update CUPS settings UI to allow USB printers to be added via discovery. This re-enables the "discovery" dialog when a user asks to add a printer; the dialog is populated with any USB printers that could not be set up automatically. (Eventually, this will also be populated with auto-discovered network printers). If the user selects a printer from the list, they are taken to the manufacturer-model dialog to select a driver. This change also alters a button in the ui. Right now, there's always an "Add printer manually" button in the manufacturer-model dialog. But this *actually* functions as a "back" button when adding a printer manually, as it takes you back to the previous dialog with the values you entered there, and, in the discovery case, this button doesn't make sense as it doesn't have previous values to take you back to. So the button has been re-texted with 'back', and now takes you to the previous dialog, whether that was discovery or manual addition. Finally, this change also fixes a few things in URI handling for usb printers in cups_printers_handler so that, when we add a printer from the manufacturer-model dialog, we get a correct URI to do so. BUG=616866 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2825153002 Cr-Commit-Position: refs/heads/master@{#471858} Committed: https://chromium.googlesource.com/chromium/src/+/2ed79b998d112e09fc22678cd4bd... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/2ed79b998d112e09fc22678cd4bd... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
