LibreOffice Online uses a rather frankensteinish mix of Poco::Timestamp and std::chrono - we should move to using the latter consistently. It would be good to move wsd/* first - since it doesn't use much Poco API anymore - although it would be good to consider whether we need some convenience wrappers around std::chrono to make it somewhat less verbose - particularly the cast to milliseconds is very ugly. Thanks !
*** Bug 107037 has been marked as a duplicate of this bug. ***
Good to do the conversion in small chunks; the easy pieces first and do them very carefully. It would be good to have a number of smaller commits tackling a particular member variable or type. Luckily std::chrono is documented reasonably well - please do research each change carefully. Thanks ! =)
Created attachment 151491 [details] changed Poco::Timestamp().epochTime() Here is a small patch I have attached. Can you tell me if this is the right way? Thank you.
Looks great to me - thanks Shivansh =) can you push to gerrit ? [ and also send a License statement to the mailing list if you havn't already ] cf. https://wiki.documentfoundation.org/Development/Developers - or do you need help with gerrit setup ?
I have pushed the commit and sent the license statement. For my next change, in wsd/FileServer.cpp and wsd/LOOLWSD.cpp, I am thinking of replacing `Poco::DateTimeFormatter::format(Poco::Timestamp(), Poco::DateTimeFormat::HTTP_FORMAT)` with `char time[50]; std::chrono::system_clock::time_point now = std::chrono::system_clock::now(); std::time_t now_c = std::chrono::system_clock::to_time_t(now); std::tm now_tm = *std::gmtime(&now_c); strftime(time, 50, "%a, %d %b %Y %T", &now_tm);` Will that be ok? If it is I am thinking of putting it into a function to avoid repetition. Thank you.
DarkByt31 committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/online/+/a243fdef88e85ec082ade9622374d20fc5a77ffb%5E%21 tdf#107038 Poco::Timestamp replacement with std::chrono
Good question - it would be nice to have a helper function to handle dates that can do that in a Util class and that returns a helpful type for that replacement - perhaps "std::string getHttpTimeNow()" or somesuch (?) =) depending on how that is used around the code though ... Thanks!
DarkByt31 committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/online/+/8e34705fe22f7efaf44619cf40685217e2216d4b%5E%21 tdf#107038 Poco::DateTimeFormatter with Util::getHttpTimeNow()
For the next change, could you help me to replace this https://opengrok.libreoffice.org/xref/online/wsd/Storage.cpp#266 Thank you.
So - we need to replace the Storage.hpp "FileInfo" class: class FileInfo { public: FileInfo(const std::string& filename, const std::string& ownerId, const Poco::Timestamp& modifiedTime, size_t /*size*/) And get it to store a std::chrono internally. Then I would tackle its methods and the callees to convert them to std::chrono too. Thanks !
I have made changes in the declaration part in Storage.hpp(changed Poco::Timestamp to std::time_t). In Storage.cpp, I couldn't find an alternate for the file.getLastModified() in line 266. We might have to use something like this https://stackoverflow.com/a/40504396/7866573 . What do you suggest to do? Thank You.
Create a helper for that that returns a std::chrono in our file utilities - it will be a wrapper of: $ man 2 stat You'll want a#ifdef __APPLE__ to use: st.st_mtimespec there - and should prolly take a file path as an argument. Thanks.
I am unable to create timestamp of the format Poco::DateTimeFormat::ISO8601_FRAC_FORMAT using std::chrono, due to the decimal in the seconds. Do have any suggestions for this? Or is it okay to go without decimal? PS- this patch is taking a lot of time.
You googled for high resolution std::chrono ? certainly it can represent high resolution time.
Created attachment 151676 [details] Poco::Timestamp replacement in Storage.hpp and Storage.cpp Yes, I gave another look to high_resolution_clock and did the required changes. There's still one thing I need to figure out. In Storage.hpp modifiedTime is of std::time_t type. Then in Storage.cpp https://opengrok.libreoffice.org/xref/online/wsd/Storage.cpp?r=0dbf9bcf#736 , I don't understand how to modify this line. Do you think I should use std::chrono::system_clock::time_point instead of std::time_t for modifiedTime?
DarkByt31 committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/online/+/7850c944e204c3a7d62029b2b319eb836c4c06d3%5E%21 tdf#107038 Poco::Timestamp replacement with std::chrono
DarkByt31 committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/online/+/22f1656e08d1cc873a523bf1f869bdd75066dd2a%5E%21 tdf#107038 Poco::Timestamp replacement with std::chrono
DarkByt31 committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/online/+/8aea22a32b28728033c4f06a79f93e63b1322151%5E%21 tdf#107038 Poco::Timestamp replacement with std::chrono
DarkByt31 committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/online/commit/dd014e7029628dd95d2026bcb4fe4a66d75785aa tdf#107038 Poco::Timestamp replacement with std::chrono
DarkByt31 committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/online/commit/ef90709ad1cb6c8cbb649425d0f0c539bbd5f83f tdf#107038 Poco::Timestamp replacement with std::chrono
Sorry for the absence. I need to ask a few things: 1) is the key in common/Crypto.cpp in UTC time or localtime? 2) if it is, will it be ok to use already existing method Util::iso8601ToTimestamp() to convert string to chrono::time_point?
I wont be able to work on this patch as I am facing problem in passing make check, which is taking a lot of time. However, If any changes are required in the current patch (https://gerrit.libreoffice.org/c/online/+/84090/4), I would be happy to apply it.