Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

choose-model.tmpl: Add warning about drivers deprecation #218

Merged
merged 2 commits into from Sep 7, 2021
Merged

choose-model.tmpl: Add warning about drivers deprecation #218

merged 2 commits into from Sep 7, 2021

Conversation

zdohnal
Copy link
Member

@zdohnal zdohnal commented Aug 16, 2021 •
edited

Since we report the warning via lpadmin, we should do the same in Web UI.

michaelrsweet
michaelrsweet requested changes Aug 17, 2021
View reviewed changes
Copy link
Member

@michaelrsweet michaelrsweet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the spirit of the lpadmin warning, can you add this warning to the "printer-added.tmpl" file instead? (and maybe make it only show if "ppd_name" is not "everywhere"?)

@zdohnal
Copy link
Member Author

zdohnal commented Aug 18, 2021

In the spirit of the lpadmin warning, can you add this warning to the "printer-added.tmpl" file instead? (and maybe make it only show if "ppd_name" is not "everywhere"?)

@michaelrsweet That would be great if we actually loaded the template somewhere :D , because we load 'set-default-options' template immediately and printer-added is never loaded :) - I'll check where we can fit it in. (or add it into printer-configured.tmpl?).

@michaelrsweet
Copy link
Member

Hmm, maybe printer-added.tmpl should be used for driver-based queues with a link/button to click to configure the queue...

@zdohnal
Copy link
Member Author

zdohnal commented Aug 18, 2021

I'm sorry, I'm not sure if I understand, when do you mean printer-added template should be used? You mean it like an already created driver based queue will have a button to configure the queue, and the template will be run after 'configuration success'?

@michaelrsweet
Copy link
Member

@zdohnal Yes, I guess I am saying you can start using the printer-added.tmpl file for a queue that has a printer driver.

@zdohnal
Copy link
Member Author

zdohnal commented Aug 18, 2021

@michaelrsweet during creating a new print queue? Your previous comment sounded like you meant it to use to an already created queue (I'm sorry, sometimes I still have problems to follow).

@zdohnal
Copy link
Member Author

zdohnal commented Aug 18, 2021

My idea can be that we can somehow put it between choose-model.tmpl and set-printer-options.tmpl. (or fall back to the original design in choose-model.tmpl...)

@michaelrsweet
Copy link
Member

@zdohnal What I'd like to see is the following:

  1. User chooses a printer (choose-device.tmpl)
  2. If it is an IPP Everywhere printer, default to the IPP Everywhere driver (choose-model.tmpl)
  3. After adding the printer:
    a. For IPP Everywhere, choose options (set-printer-options-xxx.tmpl and option-xxx.tmpl)
    b. For other drivers, show that the printer has been added (printer-added.tmpl) and that printer drivers are deprecated. Add a "set options" button to the printer-added.tmpl file.

@michaelrsweet michaelrsweet added this to the v2.4 milestone Sep 1, 2021
@zdohnal zdohnal force-pushed the webui_driver_deprecated branch 4 times, most recently from efe6061 to be6c7f1 Compare September 2, 2021 12:59
@zdohnal
Copy link
Member Author

zdohnal commented Sep 2, 2021

Ok, so the web ui should be functional in the way you proposed - if you choose non-everywhere model, printer-added.tmpl is showed, with button to go to default options.
IPP everywhere queues goes straight to default options.

Do let me know if I should update it further in some way.

michaelrsweet
michaelrsweet requested changes Sep 5, 2021
View reviewed changes
Copy link
Member

@michaelrsweet michaelrsweet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! See comments for minor feedback.

cgi-bin/admin.c Outdated
@@ -1225,6 +1236,8 @@ do_am_printer(http_t *http, /* I - HTTP connection */

cgiSetVariable("OP", "set-printer-options");
do_set_options(http, 0);
if (ppd_name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need the NULL check, and not sure we need to bother freeing it since the program just exits after the return...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... isn't ppd_name NULL if we supply a PPD file via 'Or provide a PPD file'? And we reach these blocks in that case. So I guess it goes down to whether we want to free the memory or not here.

TBH, I've added the frees because I know our coverity always shouts at me that there are memory leaks in cgi scripts, so I wanted to prevent them in the PR I've created :D . I know they're not serious, since the program ends right after and then not-freed memory is handled by OS, but it seemed like a right thing to do (I can hear my college teacher: 'free your memory which you'd allocated!'). But I don't know details about how expensive operation the freeing of memory is (I know f.e. Vim doesn't allocate on heap in some time-critical functions to prevent leaks and expensive free()s), so I'm fine with removing free()s.

cgi-bin/admin.c Outdated
@@ -1233,6 +1246,8 @@ do_am_printer(http_t *http, /* I - HTTP connection */

if (oldinfo)
ippDelete(oldinfo);
if (ppd_name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

templates/printer-added.tmpl Outdated
<H2 CLASS="title">Add Printer</H2>
<H2 CLASS="title">Add Printer {printer_name}</H2>

<P>Printer drivers and raw queues are deprecated and will stop working in a future version of CUPS.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move this after the "added successfully" message, and put it in a blockquote, e.g.:

<blockquote><b>Note:<b> Printer drivers ...</blockquote>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, I'll change it.

@zdohnal
Copy link
Member Author

zdohnal commented Sep 6, 2021

@michaelrsweet I was thinking about getting cups-filters driverless in the game too (strstr() on ppd_name looking for 'driverless', go in if NULL), WDYT?

Since it is right now another way of IPP Everywhere support we can take it into account, but I'm not sure if it is correct to have checks for stuff from other projects.

@michaelrsweet michaelrsweet self-assigned this Sep 7, 2021
@michaelrsweet michaelrsweet added enhancement New feature or request priority-low labels Sep 7, 2021
@michaelrsweet michaelrsweet merged commit b82b2fb into OpenPrinting:master Sep 7, 2021
@zdohnal zdohnal mentioned this pull request Sep 8, 2021
Closed
zdohnal added a commit that referenced this pull request Sep 8, 2021
@zdohnal
cgi-bin/admin.c: Prevent accessing ppd_name if ppd_name is NULL
92df865 
The fix provided by Alfonso Gregory, fixes regression after PR #218.
zdohnal added a commit that referenced this pull request Sep 8, 2021
@zdohnal
Merge branch 'AtariDreams-nullFix'
4454047 
cgi-bin/admin.c: Prevent accessing ppd_name if ppd_name is NULL

The fix provided by Alfonso Gregory, fixes regression after PR #218.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelrsweet michaelrsweet Awaiting requested review from michaelrsweet

Assignees

@michaelrsweet michaelrsweet

Labels
enhancement New feature or request priority-low
Projects
None yet
Milestone
v2.4
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@zdohnal @michaelrsweet