confirm

owner, add myself to CC

Accepted.

The following attachement contains my proposal for the dialog change, with some
questions. Please have a look.

Created attachment 33666 [details]
Proposed PDF Export dialog, plus a few questions.

Here some answers to the questions in the doc.

Next, a few questions.
Q1. The document navigation tab is something I didn't insert at first in the
issue requirements, do you think it will be useful to OOo users ?

I'm not sure how many people will use that tab page; on the other hand it sounds
like an easy implementation, so it won't hurt either. As long as the first
visible tab page is the one that is most commonly needed and there are not too
many tab pages, i think additional control over the output is a nice thing to have.

Q2. What impact will have the modification of the PDF export dialog on currently
existing macro (or java app) using the features as currently implemented ?

Provided that the defaults stay the same there should be no difference IMHO.

Q3. OOo help system. Should I propose the new descriptions in the PDF Export ?
Or a brief description will suffice ?

While mmp may have a better answer, i think a feature mail on the
allfeatures@openoffice.org list describing the new feature should trigger the
help people.

Q4. I need to test the new features, is there a test structure available to test
PDF output ?

Not that i know of. I usually tested new features by creating respective
documetns and viewinfg them with acrobat reader, xpdf and ghostscript.

Q5. I didn't propose it, but wouldn't it be useful for the user a couple of
button, next to the three current ones, to save/recall a named PDF Export
configuration ?

Current behaviour is that the last configuration is used. IMO that is
sufficient, but 

beppec56->pl. Further comments.
Q1. I hardcode into the code the link generation. The result is ugly, almost
useless. It would have sense if applied only to the cross-reference link, and
not to the URL ones. Presently it seems the links are not differentiated from
the cross-references. So, for now, I'll drop it.
I'll modify the dialog template as soon as I have some feedback from mmp.

For now the tab3_DocumentNav dialog in pdf-dialog-draft.odt should be ignored.

Q2. I don't intend to modify defaults.
Q3. I'll wait for mmp.
Q4. I'll implement the same.
Q5. I'll leave as it is.

Links (internal as well as external) are mostly the same, they only differ in
the kind of target (see PDFWriterImpl::emitLinkAnnotations() ), the difference
is only whether it is an URI action or has a Dest rectangle. So one could handle
them differently at that point, e.g. with a different border style which is what
i thought you had in mind. On the other hand i think it would not be logical to
treat them differently, since both are basically the same: a hyperlink.

But anyway this is independent from the rest and can be left for later time
without problems. And changing the link border style is probably not something
that would be used very often by users anyway.

beppec56->pl. Re: the links, I thought about differentiating the hyperlinks in
the original OOo document, both internal to the doc (i.e. jump to bookmarks) and
external to the doc, and the cross references. The former ones can be made stand
up at the OOo document level (hyperlink font property), whereas the cross refs
can not (or at list I did't find any way to do it). The bordering of the cross
refs in PDF doc would be a way to overcome this.
A better solution (and a more flexible one) would be to add the possibility to
provide special font properties to the cross refs at the OOo document level. I
didn't search through the issues for a request like this.

I have implemented the changes in PDFWriter_Impl, PDFWriter, PDFExport and
partly in ImpPDFDialog classes.
I'm starting the job of changing the PDF dialog to a tabbed one.

beppec56->mmp I'd like to have some comment about the dialog I proposed in the
pdf-dialog-draft.odt attachement I added in a former comment of this issue, thanks.

Actually the cross references basically are links, only they do not have an
absolute URL but a relative one. It's easier to access this feature via the
hyperlink dialog than via format->character, but after you have a cross
reference you can select it and manipulate the URL (some kind of "#<heading>" or
so) nonetheless. The hyperlink property is not a font attribute.

Nevertheless it is possible to differentiate the two; still i don't think it
would be good to differentiate the two since they are semantically the same. But
that is a matter of opinion.

beppec56->pl. I'm almost finished with the implementation, I still have some
test to run and it seems than only Acrobat Reader implements the viewer preferences.
I have not set up a ghostscript test bed, so far I have only used Acro Reader,
xpdf and kpdf to check the results.
Next, while ending my tests ( I need to check on Windows as well, at least the
PDF Acro opening), I'll start editing the PDFExport.sxw file to add the changes.

Question:
Is there a way for me to commit the code to the pdfexportimprove CWS ?
According to the explanation here:

http://wiki.services.openoffice.org/wiki/Commit_Rights

I'm at Step 1 (current status:
http://wiki.services.openoffice.org/wiki/Pending_JCAs), for now.

I can send the current code status as a diff to the CWS, if you like. I can
surely benefit of other eyes seeing the code...

you can attach the patch, and since you already send your JCA it should be no
problem for me to commit it.

beppec56->pl. The following is a patch to apply to the pdfexportimprove CWS to
update it at my current status.
Notes:
- the PDF standard dialog is still in place, it's even compiled, it's simply not
instantiated. In the final form I'll delete it, unless improving the conditioned
  compile is thought to be a better solution;
- I used the SfxTabDialog as the base class for the new dialog, but I wasn't
able to have it resized correctly without adding a hidded control to it;
- may be the way I choosed to manipulate the data beetween the Main dialog and
its pages was not for the best. I'm open to suggestions...
- to have the properties persistent across PDF Export dialog calls, the file:
officecfg/registry/schema/org/openoffice/Office/Common.xcs
should be updated as well.

Created attachment 34360 [details]
the patch to update the CWS

Contrary to my earlier comment i was bidden to wait with the commit until your
JCA has been published. I'm told that this is normally a matter of days, not the
month that already has passed, mh is working on it.

The patch looks very good, though. Some comments:
- I'll have a look at the tab page visibility problem mentioned in the
ImpPDFTabDialog constructor. 
- Reading a string from resources is always Unicode compliant since the strings
in the binary resource files are already UTF-8.
- As a tip: please construct ResId not only with the numerical ID but also with
the corresponding ResMgr (for example in the ImpPDFTabDialog constructor use
ResId( STR_PDF_EXPORT, &rResMgr ). The other ResId constructor relies on the
last ResMgr used, so you may run into trouble later if you (or someone) adds
another string from another ResMgr.

> Reading a string [snip...]the binary resource files are already UTF-8
I don't understand what you are referring to.

> As a tip: please construct ResId [snip...]
I'll revise the code and add the resource manager where missing.

Re: JCA, let me know if you need to have it resent, I had a look at it and it 
appears that my address (handwritten) is not clearly readable.

I was referring to the comment in the patch:
+//FIXME: check if the string operation is Unicode compliant

Since the operation seems to be reading a string from the resource, i just
wanted to confirm that this is unicode compliant.

I have applied the patched and played a little, works like a charm. I hope that
the JCA situation is resolved soon now, so i can commit this.

Some further thoughts:

The viewer preferences tab page is a little crowded (well, the general tab page
is of course also, since the original export dialog was crowded to begin with),
maybe we can do some more pages, now that we actually have a tab dialog. I'd
suggest to begin with making the viewer preferences page two pages. I think
everything above the "Viewer preferences" FixedLine would be a good candidate to
go on an own page, which would remove the FixedLine itself also.  Actually this
is where input from user experience would come in helpful (mmp: hint, hint :-) )

Next: GroupBoxes are sort of out of style, the dialog would fit better with the
rest of the office UI if they were changed to FixedLines; thoughout the office
dialogs FixedLines have taken over the role of the GroupBoxes previously in use.

Also the viewer preferences should probably be made persistent the way the
general values are.

beppec56->pl.
OK, I'll divide the preferences page, but two radio buttons on different groups
are connected:

Open Mode: Full screen radio button toggles the enable status of all Non Full
Screen Mode radio buttons.

this goes according to PDF specification. Of course the check is done in the
PDFWriterImpl class as well before outputting it. Should this two stay together
? I suppose that's not easy (and not very usefull) enabling/disabling controls
across tab pages, I'll try different solutions to see how it shows.

I'll change the group boxes to fixedlines.

I've already made the preferences persistent modifying the:

officecfg/registry/schema/org/openoffice/Office/Common.xcs

file accordingly, should I provide a patch for this as well ?

During this week-end I think I'll have most of the changes done. ASA the JCA
matter gets sorted out, my proposed course of action: commit the patch I'll next
provide updated with my last changes, update the CWS to 2.0.2 milestone (if all
this is possible).

Currently adding the officecfg project to the CWS, yes please add a patch to the
Common.xcs.

You're of course right about the radio button / fullscreen problem. As you said
it would be more sensible to leave the Opening Mode and Non Fullscreen Mode
options together. So what do you think about about keeping "Opening Mode" and
"Non Fullscreen Mode" on one tab page and "Opening action", "Direction Control"
and "Viewer Window Control" on the other ?

I've already changed the dialog as you propose. I attach the two screenshots of
the now two tabs (the first one stays the same).
I have some code polishing to do then it will be finished, but for the ooo help
system that now is broken, even for the older PDF options.
I'll add Common.xcs to the single patch I'll provide ASA the polishing is finished.

Created attachment 34468 [details]
screen shot of first tab page (second absolute)

Created attachment 34469 [details]
screen shot of 2nd tab page (3rd absolute)

Looks very good to me. If it's fine by you we should stay with this layot. In
the meantime i'll try (again) to get info about what has happened to your JCA.

beppec56->pl.
The attachement is the patch containing all the changes I worked out to
implement this issue, I added the Resource Manager changes you outlined in a
former comment, along with some cleanup and the tab layout I last proposed.
It contains the modification to Common.xcs needed to make the new filter
properties persistent.

Someone ought to test the other filters in the filter module (I touched the
definitions) to see if any regression was introduced there (I don't think so,
but you never know...).

The help is completely broken for the currently 2.0.1 available PDF Options,
even if the help is there, it appears that the help IDs are automatically set
differently within the tab dialog, may be a change in some of the help list is
sufficient, but I don't know how to do it (yet).

Still to to:

- check if the macro basic works out of the box with the new properties, I'll
try it as the next test step;
- UI should have a look at this (I believe that you where able to compile it). A
question in this realm can be: Reset button does not changes the value back to
the ones there when the tab was first opened/selected, should the text on it be
changed, as well as the help id ?
- advertise the change in interface in gsl-announce (it's the right list, isn't
it ?)

Created attachment 34526 [details]
patch to apply to pdfexportimprove CWS

Will compile the patch and try to grab hold of mmp. Regarding the help system i
have no idea, i'll have to inquire. Regarding the reset button: you are right,
something like "Defaults" or "Reset to defaults" might be clearer.

Did the compile and everything seems too work flawlessly, great job. :-) I could
not get hold of mmp but will do so next week, that will IMHO not be an issue.

I asked again about what happened to your JCA and hope that we will have the
answer soon. I can only say I'm really sorry that this took us so long.

Also you were right why the helpid's don't work anymore; they are automatically
calculated by the window hierarchy unless they are made explicit in the .src
file and the controls on the general tab page are now are subcontrols of a
tabcontrol which offsets the helpid by 0x20000000L (see
tools/source/rc/resmgr.cxx: ResMgr::GetAutoHelpId). I don't know how this would
be handled best, i'll have to ask around a bit.

Just talked to mmp, he mentioned some things:

- on the second tab page in the second radio group the last Radiobutton is more
distant to its predecessor than it should
- The text "Thumbnails images" should be either "Thumbnail images" or
"Thumbnails" (personally I think just "Thumbnails" is better since it is shorter).
- the "Action to be performed when the PDF document is opened" is a little
longish, also all points below it begin with "First page". Perhaps we can find a
more concise form; e.g. like
   "On opening the first page displays"
   "  - at the default zoom factor"
   "  - fitting page height"
   "  - fitting page width"
   "  - fitting visible objects"

set target and spec URL

The spec is "work in progress". Many strings are subject to change to conform
with Acrobat's dialog.

beppec56->mmp. I've modified the tabs as per spec draft.
I attached two bitmap of the new tabs, please have a look.

Created attachment 34617 [details]
tab 2 (2nd version)

Created attachment 34618 [details]
tab 3 (2nd version)

I've implemented the Page Layout behaviour, well, it was really needed.
ASA I've changed the filter property names to more adapt ones I'll send a patch.

->mmp. A question regarding the Reset button: it sets the values on the current
tab to the Openoffice default, should a different title be used ?

Good news: i talked to mh and he told me that i can check in your patch now; the
JCA still hasn't turned up, but either it will or mh or st will contact you
about resending it. Either way we can proceed now with checking in. If you'd be
so kind as to apply a patch with the latest changes i'll commit that ASAP.

After querying around somewhat the solution to the help and helpid issue goes
like this: when we're done you or i will write a feature announcement with the
help and ui checkmarks checked; this will trigger the help department to update
the help and helpid for the dialog. In fact there does not seem a way for us do
to anything before the dialog changes get integrated into the master.

The attached patch contains all the changes to the spec draft.

Created attachment 34685 [details]
the patch to update the CWS to the latest spec draft

committed to CWS pdfexportimprove

currently adding to section 5 of the spec (configuration items)

beppec56->pl. Yesterday night I sent the patch and today I realized there is a
typo in one of the string, pointed out in the updated spec.
The string is the first fixline of the 3rd tab:
now reads:   Window Options
should have been: Window options
I gather that the patch was integrated, how can I send the correction ? Or
should I wait for further comments ?

beppec56->mmp. String at line 59 of spec reads: Resize window to inition page,
shouldn't it be Resize window to initial page ?

oops, did an involuntary reassignment, sorry

corrected "Window Options" to "Window options", checked in CWS pdfexportimprove

BTW. the patch was not integrated (that's when it will go into the master), i
just committed it to our CWS branch.

Yes, there was a typo in "Resize window to initial page"
Another typo is in the sting "Display document title" // missing l

I added the string tables to the spec and will add the mnemonics now.

It is also my opinion that we do not need the Reset feature. Tab 2 and 3 are
really easy to grasp. And we had no "Reset" for the first panel before anyway.

- mnemonics in spec are final.
- The Reset button should go away.
- Note the section on Open Issues
BTW - thanks for the good and fast work. Matthias

->pl.
New patch, provides the lastest change from spec:
- Final English strings & memonics, pendig variations indicated in spec;
- removed Reset button;
- verified the default, all ok but one, see below, changed Config.xls accordingly.
I applied the patch successfully to oob680_m5 tag.

->mmp.
I suppose that 'marked' in the tables describing the interface elements means
'Ooo default'; so in the case of 'Display document title' in tab 3 there is
a difference from the configuration (section 3 of spec) and the table:
- table 3 -> not selected (def. false);
- Section 3 (config) -> default true;
which one ? I choose the one in the configtable (def.->false), if it's correct
the Configuration should be corrected.

Re 'Main reading order for text' remarks in the spec, both the buttons,
considerations: this was needed without 'Page layout' selection, it could be
deleted and in 'Page layout' we may have: 'Continuous facing even page first'
(currently 'Continuous facing' ) and a new button: 'Continuous facing odd page
first'.

Created attachment 34716 [details]
the patch !

commited the latest patch, but please send further patches against the cws
branch; the patch would not apply of course since the previous patch already was
applied. Please work on the cws branch in cvs (branch label is
cws_src680_pdfexportimprove). Sorry, i should have thought of that problem
earlier. If you cannot convince cvs to get you that branch (cvs at collab being
flaky sometimes), please mail to me, i can sent you the corresponding source
modules directly.

I also took the liberty of removing the mbResetAlreadyCalled members we don't
need anymore.

Re: 'Display document title'
yes you are right. The default should stay as "Display Title"

Re: "Reading order for text"
Now I understand this option. Thanks. Please remove the options from the 3rd
page and add a new check box on the second that depends on the last Option
"cont-facing"; visually indented.

Strings are final.
Please have a look into the spec and let me know if anything is missing.

->pl.
I'm going to attach the latest patch, lastest changes from spec (March 10th vers):
- Updated English strings;
- Added German strings;
- changed Config.xls to reflect the new tab layout (name of properties more
suited to it), so the Configuration section of the spec should be changed as
well, see below.
The patch is incremental to CWS (please check if I did right).
I patched also the current oob680_m5 tag for myself, it seems to run just fine.

->mmp.
I changed the dialog tabs for the reading order of text.
With the current spec (as of March 10th) implemented (in the patch above):
- Continuous facing selected, First page is left unselected: standard behavior
for L2R text, page 1 on the right, then page 2+3, 4+5, etc.. last page (if even)
is on the right;
- Continuous facing selected, First page is left selected: behavior for R2L text
(I suppose), page 1 on the left, then page 3+2, 5+4, etc.. last page (if even)
is on the left
Is this all right ?

In the Open Issues section I saw "... image quality?" (3rd line), is it
something related to the General Options tab, e.g. the quality value currently
default at 90% ?

Some updates were needed for the spec, I edited the spec by updating the tab
screenshots to the latest, updated the text. The new spec version has changed
registered in it, I'm going to attach it.

I don't see anything missing, but the initial view page number, as seen in
Illustration 2 in spec, do you think is needed in OOo ? In my experience with
PDF generation I've always generated them to open on the first page, but it's
only me...

Created attachment 34796 [details]
latest patch against CWS

Created attachment 34797 [details]
updated spec, changes registered in it, OOo flavor.

spec update with Giuseppe's changes.

Regarding the "First page left" option: I dunno. Philipp or Giuseppe, what is
really happening in Acrobat if this option is set?

Created attachment 34811 [details]
pdf example cont.facing, First ... not selected

Created attachment 34813 [details]
pdf example cont.facing, First ... selected

Yes, we do not support "Open on page x". I am not  convinced that this feature
is essential.

->mmp. Please have a look at the two examples. Both should open facing. The L2R
example (1st one) has the fist page on right and the facing pages are sequential
from left to right: First on left not selected.
The 2nd example has the first page on left and the facing pages are sequential
from right to left: First on left selected.
So this selection is meant as a helper for readers of PDF document written
mainly in R2L text (e.g. Arab, Hebrew).

->mmp.
Re: "Open on page x". I think the same. If in the future some users ask for it,
it can be easily implemented.

Checked in the latest patch.

pl->mmp: "Benutzungsschnittstelle" ? Shouldn't that be "Benutzeroberfläche" ?
And as well "Benutzungsoberflächenoptionen" "Benutzeroberflächenoptionen" ?

"Schnittstelle" vs "Oberfläche" > I'll chekc with LingTool. Might take some time
"Benutzer-" vs "Benutzungs-" > Hey it is the surface of the app rather than the
user's surface.

pl->mmp: Since you seem to like the acrobat dialog so much, just have a look
there in the german version (hint: it's in the spec already ;-) ).

Spec update:
Help button is bottom left
Changes in German strings “Benutzungs-†to “Benutzer-â€

Open issue: "First page is left"  -- Thanks Guiseppe for the attachments. I
suppose this is the original R2L idea.
I'll update the spec and we decide if this makes sense this time.

spec update (I think we are nearly done)
"First page is left" depends on CTL settinng. See spec for details.

->mmp. In the spec the help button was moved to the bottom left. Presently I'm
using a class derived from a sfx2 element (SfxTabDialog), on which the standard
button are already there. It's the same dialog as the one used for Format |
Character, hence the current help button position, with all the buttons on the
left. It doesn't seem easy for me to implement the help btn on the other side,
in the 2.0.3 timeframe I mean. Besides it will be the only tabbed dialog in OOo
to have the help on the other side of the dlg.

Oops, in the former comment I wrote:
"
hence the current help button position, with all the buttons on the
left
"
Should have been:
"
hence the current help button position, with all the buttons on the
right
"

Good point. For now the Help button stays where it is: Far right. No need to
touch sfx for this patch.

Hi Giuseppe, can you please take a look at i43362? Is there an easy, straight
forward solution for that?
Otherwise, I suppose we are done. Thanks
Matthias

One thought on issue 43362: the jpeg issue is a non issue; if the original is
jpeg it already gets put into the pdf in its original form; that means if
uncompressed images is selected the behaviour is already as wanted. The
substitution issue is not really an easy fix since this can only be done for
western text, so one would have to make this decision on the fly when encoding
the font subsets to be put into the PDF.

ok - from my point of view we are done.

->mmp.
Re: help btn: so it stays as it is, good.
Re: i43362: I had only a very quick glance at it, but it appears difficult to
implement to close that issue as well. What I can do is to clean up the emitted
PDF file, for example changing \r\n -> \n) and deleting the blanks not needed,
according to PDF syntax. I did that on one of the first testings, but I only get
some 8 to 10% size reduction on big files (500k up). In files as the one
indicated in that issue the improvement is surely less.
This appears quick and simple to implement, even if the PDF generated, if opened
with a text editor for debugging purposes, will be difficult to interpret. Let
me know if you think it worthwhile.
 I'm thinking about other PDF things (e.g. i12626 seems challenging...), so I'm
going to add 43362 to my list of 'preferred' PDF issues.

8% is a lot. I would recommend to keep the linebreaks untouched, but the white
space should be considered to be eliminated. What is the idea?
a) create pdf and clean it up, or
b) create clean pdf right away
Performance is always in issue with OOo -- be aware of the tradeoffs.

Anyway this should be discussed in a new task.

->mmp option b) above, may be there are other 'little things' to clean up.
re: end-line termination, PDF spec reccomands a single LF, the CR/LF it's just
doubling it up. Ghostscript uses LF only and the lines are surely more
populated, e.g with less LF in between.
I'll strip out only spaces, but I doubt to reach 8%, some percent yes though.

->pl.
Latest patch: latest version of German strings, CTL behavior implemented (see spec).
The patch doesn't contain blank cleanup discussed above.

Created attachment 34881 [details]
latest patch against CWS

Overnight I built OOo 2.0.2 with some other languages enabled, and I discovered
that part of the translation process already done on the first tab is now
broken. It appears that the file:
filter/source/pdf/localize.sdf
has some of the string referred only to the symbol of the control, and these are
translated, others strings are referred to the dialog symbol+control symbol and
these are gone.
Is there something I can do, or the traslation process must be re-run somehow ?

As far as i know the translation process is like the help process an automated
one that we cannot influence very much.

committed the latest patch. Thanks for your good work !

I opened issue 63199 regarding the whitespace.

You are all welcome. Mind if I forward the patches for 2.0.2 to the ooo-build
people, of course pointing out that this can broke help, translation, PCs, HDU
and all that ?

1. these are the fruits of your work
2. it's open source
3. ooo-build will get the patches anyway through the master version

so, by all means, please send them if you like to anybody you want to :-)

Hi Giuseppe,
- can you please activate Tagged PDF by default (spec row 75)
- Export Notes should be disabled by default   (row 77)
- and BTW in row 74 is a "." too much.
thanks
Matthias

Stop it, tagged PDF should NOT be the default. Saving 1% by optimizing
whitespace and then losing 50% by producing tagged PDF can certainly NOT be our
goal.

Are you kidding? 50% increase in file size for added tagged PDF??
If you are nearly right, then I take back my own suggestion
(No notes by default should not increase the file size, does it? :-)

As always the exact increase by tagged PDF is dependent on the actualy content.
However for tagged PDF not only the tags themselves in the content stream have
to be emitted (stuff like /P << /MCID someid >> BMC <normal content> EMC) but
also the structural hierarchy has to be emitted, that is for each structural
element an own object like e.g. for a paragraph

50 0 obj
<< /Type /StructElem
     /S /Para
     /P 49 0 R
     /Pg 2 0 R
     /K [ 2 ]
>>
endobj

Moreover this is extra space that cannot be compressed in PDF-1.4 (would need to
emit 1.5 for that). So yes, tagged PDF is indeed considerably larger than
untagged PDF. Actually this is one of the places where stripping out whitespace
can save us a nice little something because all these extra objects don't have
content streams and so have currently a larger proportion of unnecessary whitespace.

+1 for not having tagged pdf as default. There is no benefit for normal users as
they won't even know tagged pdf at all.

If I don't see other comments, I'm going to do:
- Export notes def -> false;
- align the text as in row 74 of spec
Changing both config and code accordingly.

Sven, is TaggedPDF related to the export of bookmarks/table of contents? I
suppose so, and this is not only for the sake of A11n.

mmp: no TOC and bookmarks are not dependant on tagged PDF. The TOC makes up
another structural element in tagged PDF but can be added independantly.

->pl. Latest patch follows, contains changes in config.
The patch before this (id=34881) doesn't seem into the CWS, I build this against
the CWS I see now (around 23:14 CET).

Created attachment 34909 [details]
latest patch, comprehend id=34881 (pdfexp6.patch), this is pdfexp7.patch

committed patch 7. The seemingly missing patch6 is most likely a caching issue
with cvs; this is not the first time somebody got hit with that. Sometimes it
takes many hours for the commit to be seen somewhere else :-(

->pl: the following patch corrects some glitch in the CWS.

Created attachment 34938 [details]
patch, may be final, this is pdfexp8.patch

committed

reassign for verification

pl->hi: please verify in CWS pdfexportimprove

I don't try all possible combination but it looks good in single settings = OK

Created attachment 35389 [details]
test plan to be included to spec

Looks still good in 680m169

*** Issue 61035 has been marked as a duplicate of this issue. ***

*** Issue 44917 has been marked as a duplicate of this issue. ***