On 04/06/12 14:01, Brad Sowden wrote: > Hi, > > This patch converts a SvStringsISortDtor to a vector and maintains > equivalent functionality.
hi Brad, thanks for the patch, i've pushed it to master. > Instead of sorting strings upon each insertion all strings are now added > and then a single sort and "unique" are applied (SvStringsISortDtor > behaves like a SET with regards to insertion, which is why "unique" is > applied). SwEditWin::ShowAutoTextCorrectQuickHelp() is the only place > where strings are added to the vector. > > The change in behaviour from keeping the first inserted version of a > string to keeping the first version post-sort (case-insensitive ASCII > comparison) should have no material impact as the strings retrieved from > SwAutoCompleteWord are already unique (case-insensitive ASCII > comparison) and the capitalization of the string is generally changed > anyway to match the capitalization of the word to be auto-completed. > Also, there appears to be no logical reason to store the first inserted > version of a string over the first version post-sort. hmmm... makes sense. > * In previous patches that I've seen on the list it appears ok to > convert a vector of String pointers to simply a vector of String values > so I've done the same. yes that's a nice improvement. > * I used a pointer to the vector as the move() function previously > copied and cleared all elements whereas a pointer swap would suffice. ah that's why, i had wondered.... would it be possible to use std::swap on the vector itself rather than the pointer? i could imagine that being implemented efficiently, and it would allow the vector to not be a pointer, which is simpler. also (but that was a pre-existing condition, not really your fault) member names should start with "m_" so it's easy to grep for member access. > * I've put a TODO in the code to replace the ASCII only case-insensitive > sort (existing limitation). > > - make + make dev-install successful that's a good start, please try to use "make check", which is a one-stop command that does everything. it probably wouldn't have found a problem in this case, as i doubt there's a good test for autotext, but in general it's a good idea. > - functionality tested ok > - licence statement on file regards, michael _______________________________________________ LibreOffice mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/libreoffice
