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

ppdMarkDefaults does not clear the marked member of existing choices #3642

Closed
michaelrsweet opened this issue Aug 14, 2010 · 7 comments
Closed

ppdMarkDefaults does not clear the marked member of existing choices #3642

michaelrsweet opened this issue Aug 14, 2010 · 7 comments
Labels
Milestone

Comments

@michaelrsweet
Copy link
Collaborator

@michaelrsweet michaelrsweet commented Aug 14, 2010

Version: 1.4.4
CUPS.org User: benzea

If one looks into the ppd_mark_option function, there is an if clause:
if ((oldc = (ppd_choice_t )cupsArrayFind(ppd->marked, c)) != NULL)
however, this does *not
return the old choice. It only removes it if the new choice is equal to the old choice.

Note that trunk is also affected AFAICT.

The symptoms in GTK+ are that basically for a PICK_ONE option multiple choices are selected. For example, I got a printer that can do no stapling, normal stapling and saddle stich stapling. Now, after having selected normal stapling and saddle stich stapling at one point, GTK+ will mark all of the output tray options as invalid.

I tried to add the following code before Marking an option in the GTK+ conflict handling code, and it works fine:
for (i = 0; i < ppd_option->num_choices; i++)
{
if (ppd_option->choices[i].marked)
{
ppd_option->choices[i].marked = 0;
cupsArrayRemove(ppd_file->marked, &ppd_option->choices[i]);
}
}

It is probably better to do the same thing inside cups instead though :-)

@michaelrsweet
Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Aug 16, 2010

CUPS.org User: mike

If you look at the comparison function for the marked array, you'll find that it only looks at the option name, not the choice name. The only way that this will fail is if the ppd_choice_t element's option pointer is wrong.

Since the unit test for the PPD functions checks that the PickOne semantics are honored, I would say you have either a distribution-specific bug in CUPS or a memory smasher in GTK+/GNOME.

@michaelrsweet
Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Aug 20, 2010

CUPS.org User: benzea

Uhm, OK, can't I reopen the bug?

Thing is the bug exists, it is in another part of the code though.

If you look at ppdMarkDefaults, you find the following:
for (c = (ppd_choice_t *)cupsArrayFirst(ppd->marked);
c;
c = (ppd_choice_t *)cupsArrayNext(ppd->marked))
cupsArrayRemove(ppd->marked, c);

Now, what is missing there is that c->marked is set to FALSE. Which means that while the option is removed from the marked array, its internal state is not changed.

I can provide a PPD and some C code that reproduces the bug if desired.

@michaelrsweet
Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Aug 20, 2010

CUPS.org User: mike

(FWIW, ordinary users cannot reopen bugs due to a long history of abuse...)

Reopening bug and changing the title to reflect the true issue.

That said, CUPS already provides an API to do very efficient constraint handling that GNOME should probably be using (cupsResolveConflicts) that supports both the traditional Adobe 2-way UIConstraints/NonUIConstraints and the newer N-way cupsUIConstraints/cupsUIResolver keywords.

@michaelrsweet
Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Aug 20, 2010

CUPS.org User: mike

P3 since our internal implementation no longer uses the marked member.

@michaelrsweet
Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Aug 20, 2010

CUPS.org User: benzea

Hm, I am just trying to get the printer here to work fine with linux right now ...

What GTK+ does it that it calls ppdMarkDefaults, and then sets all the options from the UI with ppdMarkOption. After this, a call to ppdConflicts and iterating over the PPD options to check ->conflicted.

cupsResolveConflicts might be interesting, the question would also be how to solve this UI wise though (and I don't really want to get into this too deep).

@michaelrsweet
Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Aug 31, 2010

CUPS.org User: mike

Fixed in Subversion repository.

@michaelrsweet
Copy link
Collaborator Author

@michaelrsweet michaelrsweet commented Aug 31, 2010

"str3642.patch":

Index: cups/mark.c

--- cups/mark.c (revision 9277)
+++ cups/mark.c (working copy)
@@ -461,7 +461,10 @@
for (c = (ppd_choice_t *)cupsArrayFirst(ppd->marked);
c;
c = (ppd_choice_t *)cupsArrayNext(ppd->marked))

  • {
    cupsArrayRemove(ppd->marked, c);
  • c->marked = 0;
  • }

/*

  • Then repopulate it with the defaults...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.