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.

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

Support retrieving localizations from PPDs with cupsLocalizeDestValue() #546

Closed
wants to merge 1 commit into from

Conversation

Plombo
Copy link
Contributor

@Plombo Plombo commented Nov 17, 2022

The first commit in this pull request makes cupsd include "printer-strings-uri" when listing its supported printer attributes. This makes the cupsLocalize* functions work when connected to the CUPS server (rather than directly to a printer). As a convenient side effect, it also makes ippeveprinter properly advertise the attribute.

The second commit updates .strings file creation to use localized names from PPDs when available for values of the "media-type", "media-source", "output-bin", and "finishing-template" attributes. This allows clients to retrieve localized names for these attributes using cupsLocalizeDestValue() instead of the legacy PPD API. I've only tested it with "media-type", but the other three should work as well.

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.

We need to be very careful about increasing the amount of data that is loaded from PPDs, as that can have a negative effect on cupsd performance and memory usage on servers.

If we take a change like this, I would like to defer it to 2.5.x (the next feature release) since a) that release has a focus on localization improvements and b) it will give us more time to tune things for better performance (like maybe separating out the strings file generation/caching...)

@@ -4027,7 +4027,7 @@ load_ppd(cupsd_printer_t *p) /* I - Printer */
ippAddStrings(p->ppd_attrs, IPP_TAG_PRINTER, IPP_CONST_TAG(IPP_TAG_KEYWORD), "job-creation-attributes-supported", sizeof(job_creation_print) / sizeof(job_creation_print[0]), NULL, job_creation_print);
}

if ((ppd = _ppdOpenFile(ppd_name, _PPD_LOCALIZATION_NONE)) != NULL)
if ((ppd = _ppdOpenFile(ppd_name, _PPD_LOCALIZATION_ALL)) != NULL)
Copy link
Member

Choose a reason for hiding this comment

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

This was actually changed a while back because it caused a huge startup delay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see; that does make sense. Would using _PPD_LOCALIZATION_DEFAULT instead be a significant improvement or almost as bad?

Copy link
Member

Choose a reason for hiding this comment

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

@Plombo It's just as bad - any localization slows the loading of the PPD.

@michaelrsweet michaelrsweet added this to the v2.5 milestone Nov 18, 2022
@michaelrsweet michaelrsweet added enhancement New feature or request priority-medium labels Nov 18, 2022
@Plombo
Copy link
Contributor Author

Plombo commented Nov 18, 2022

Upon further investigation, I think I was wrong about the cupsLocalize* functions needing the first commit in this pull request, but I wasn't wrong about ippeveprinter needing it. Given that the performance/memory concerns seem to be limited to the second commit (the one involving PPD localizations), it probably makes sense to split the first one out into its own pull request (which could be merged sooner) and let this one be dedicated to the second. I'll get going on that.

With this, the cupsLocalize* functions will pass through localizations
defined in PPDs.

v2: Use _PPD_LOCALIZATION_DEFAULT instead of _PPD_LOCALIZATION_ALL to
mitigate the impact on performance and memory usage.
@Plombo
Copy link
Contributor Author

Plombo commented Nov 18, 2022

I've now done the above, splitting the first commit into a separate pull request (#551). I also updated the PPD localization commit to use _PPD_LOCALIZATION_DEFAULT instead of _PPD_LOCALIZATION_ALL.

@michaelrsweet
Copy link
Member

Yeah, we will need to do this a different way... I'm looking at creating a single set of localizations for all printers so that we can just serve up those instead of doing separate files per-printer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority-medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants