Bug 107038 - online: Poco::Time* replacement with std::chrono
Summary: online: Poco::Time* replacement with std::chrono
Status: NEW
Alias: None
Product: LibreOffice Online
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: low trivial
Assignee: Not Assigned
URL:
Whiteboard: target:6.3.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
: 107037 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-04-08 20:31 UTC by Michael Meeks
Modified: 2019-05-25 18:41 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
changed Poco::Timestamp().epochTime() (1.33 KB, patch)
2019-05-17 18:53 UTC, Shivansh
Details
Poco::Timestamp replacement in Storage.hpp and Storage.cpp (6.86 KB, patch)
2019-05-25 18:41 UTC, Shivansh
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Meeks 2017-04-08 20:31:06 UTC
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 !
Comment 1 Michael Meeks 2017-04-08 20:31:27 UTC
*** Bug 107037 has been marked as a duplicate of this bug. ***
Comment 2 Michael Meeks 2019-05-06 13:57:23 UTC
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 ! =)
Comment 3 Shivansh 2019-05-17 18:53:45 UTC
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.
Comment 4 Michael Meeks 2019-05-18 10:31:40 UTC
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 ?
Comment 5 Shivansh 2019-05-18 18:36:58 UTC
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.
Comment 6 Commit Notification 2019-05-18 21:43:31 UTC
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
Comment 7 Michael Meeks 2019-05-19 08:52:01 UTC
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!
Comment 8 Commit Notification 2019-05-20 09:53:38 UTC
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()
Comment 9 Shivansh 2019-05-22 17:04:39 UTC
For the next change, could you help me to replace this https://opengrok.libreoffice.org/xref/online/wsd/Storage.cpp#266

Thank you.
Comment 10 Michael Meeks 2019-05-23 19:32:13 UTC
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 !
Comment 11 Shivansh 2019-05-23 20:35:57 UTC
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.
Comment 12 Michael Meeks 2019-05-23 20:51:24 UTC
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.
Comment 13 Shivansh 2019-05-25 10:18:34 UTC
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.
Comment 14 Michael Meeks 2019-05-25 10:56:44 UTC
You googled for high resolution std::chrono ? certainly it can represent high resolution time.
Comment 15 Shivansh 2019-05-25 18:41:46 UTC
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?