Unity 8

Merge lp:~nick-dedekind/unity8/StrFTimeFormatter into lp:unity8

StrFTimeFormatter
Merge into trunk

Proposed by Nick Dedekind on 2013-10-23

Status:	Merged
Approved by:	Michał Sawicz on 2013-12-18
Approved revision:	473
Merged at revision:	598
Proposed branch:	lp:~nick-dedekind/unity8/StrFTimeFormatter
Merge into:	lp:unity8
Prerequisite:	lp:~larsu/unity8/lp1236413
Diff against target:	239 lines (+67/-11) 11 files modified plugins/Unity/Indicators/CMakeLists.txt (+0/-1) plugins/Unity/Indicators/Messaging/qml/SimpleTextMessage.qml (+2/-1) plugins/Unity/Indicators/Messaging/qml/SnapDecision.qml (+2/-2) plugins/Unity/Indicators/plugin.cpp (+0/-2) plugins/Utils/CMakeLists.txt (+1/-0) plugins/Utils/plugin.cpp (+3/-0) plugins/Utils/timeformatter.cpp (+30/-1) plugins/Utils/timeformatter.h (+15/-3) tests/plugins/Unity/Indicators/CMakeLists.txt (+0/-1) tests/plugins/Utils/CMakeLists.txt (+1/-0) tests/plugins/Utils/timeformattertest.cpp (+13/-0)
To merge this branch:	bzr merge lp:~nick-dedekind/unity8/StrFTimeFormatter
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2013-12-18
Michał Sawicz			Approve on 2013-12-18
Lars Karlitski (community)		2013-10-23	Approve on 2013-10-24
Review via email: mp+192343@code.launchpad.net

Commit message

Added parser for strftime in TimeFormatter.
Moved TimeFormatter to Utils plugin.

Description of the change

Added parser for strftime in TimeFormatter.

PS Jenkins bot (ps-jenkins) wrote on 2013-10-23:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity8-ci/1480/rebuild

review: Needs Fixing (continuous-integration)

Lars Karlitski (larsu) wrote on 2013-10-23:

This patch doesn't check the return value of strftime() and might thus pass an uninitialized buffer into QString().

According to strftime(3), the return value of that function is 0 when the resulting string is longer than the passed in buffer. However, it can also be 0 when the resulting string is legitimately empty. For example, when the format string is "".

I think the best solution is to prepend a " " to the format string. That way, the return value is never smaller than 1, unless there's an error. And then simply return QString(buffer + 1).

You could also use g_date_time_format(), which supports all of the POSIX formats as well.

review: Needs Fixing

Nick Dedekind (nick-dedekind) wrote on 2013-10-24:

> This patch doesn't check the return value of strftime() and might thus pass an
> uninitialized buffer into QString().
>
> According to strftime(3), the return value of that function is 0 when the
> resulting string is longer than the passed in buffer. However, it can also be
> 0 when the resulting string is legitimately empty. For example, when the
> format string is "".
>
> I think the best solution is to prepend a " " to the format string. That way,
> the return value is never smaller than 1, unless there's an error. And then
> simply return QString(buffer + 1).
>
> You could also use g_date_time_format(), which supports all of the POSIX
> formats as well.

GDateTime it is.
Renamed to GDateTimeFormatter as well.

PS Jenkins bot (ps-jenkins) wrote on 2013-10-24:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity8-ci/1487/rebuild

review: Needs Fixing (continuous-integration)

Lars Karlitski (larsu) wrote on 2013-10-24:

Great, thanks.

review: Approve

PS Jenkins bot (ps-jenkins) wrote on 2013-11-07:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity8-ci/1594/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2013-11-07:

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity8-ci/1600/rebuild

review: Needs Fixing (continuous-integration)

Michał Sawicz (saviq) wrote on 2013-11-21:

What did we decide in the end on that? Should this come pre-formatted from the indicator?

review: Needs Information

Nick Dedekind (nick-dedekind) wrote on 2013-11-28:

> What did we decide in the end on that? Should this come pre-formatted from the
> indicator?

No, we would need to subscribe to tz changes and update each event in the indicator service and update the actions. The indicator should keep processing to a minimum (times are read in as unix time).

Nick Dedekind (nick-dedekind) wrote on 2013-11-28:

> What did we decide in the end on that? Should this come pre-formatted from the
> indicator?

I think this is still the correct way to do it. I'm not sure where the indicator gets it's format string from, but perhaps it should be a gsettting somewhere.

PS Jenkins bot (ps-jenkins) wrote on 2013-12-10:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/1851/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2013-12-11:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/1864/rebuild

review: Approve (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2013-12-13:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/1897/rebuild

review: Approve (continuous-integration)

Michał Sawicz (saviq) wrote on 2013-12-18:

Ohkay ;P

review: Approve

PS Jenkins bot (ps-jenkins) wrote on 2013-12-18:

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) on 2013-12-18:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Mohammad Rakibul Hasan

Nick Dedekind

Unity Team

1	=== modified file 'plugins/Unity/Indicators/CMakeLists.txt'
2	--- plugins/Unity/Indicators/CMakeLists.txt 2013-10-16 19:48:57 +0000
3	+++ plugins/Unity/Indicators/CMakeLists.txt 2013-12-13 13:24:49 +0000
4	@@ -36,7 +36,6 @@
5	unitymenumodelcache.cpp
6	unitymenumodelstack.cpp
7	visibleindicatorsmodel.cpp
8	- timeformatter.cpp
9	)
10	add_definitions(-DUNITYINDICATORS_LIBRARY)
11
12
13	=== modified file 'plugins/Unity/Indicators/Messaging/qml/SimpleTextMessage.qml'
14	--- plugins/Unity/Indicators/Messaging/qml/SimpleTextMessage.qml 2013-10-16 19:48:57 +0000
15	+++ plugins/Unity/Indicators/Messaging/qml/SimpleTextMessage.qml 2013-12-13 13:24:49 +0000
16	@@ -21,6 +21,7 @@
17	import QtQuick 2.0
18	import Ubuntu.Components 0.1
19	import Unity.Indicators 0.1
20	+import Utils 0.1 as Utils
21
22	HeroMessage {
23	id: __heroMessage
24	@@ -50,7 +51,7 @@
25	opacity: 0.0
26	enabled: false
27
28	- TimeFormatter {
29	+ Utils.TimeFormatter {
30	id: timeFormatter
31	time: __heroMessage.time
32	format: "hh:mm - MMM dd"
33
34	=== modified file 'plugins/Unity/Indicators/Messaging/qml/SnapDecision.qml'
35	--- plugins/Unity/Indicators/Messaging/qml/SnapDecision.qml 2013-10-16 19:48:57 +0000
36	+++ plugins/Unity/Indicators/Messaging/qml/SnapDecision.qml 2013-12-13 13:24:49 +0000
37	@@ -20,8 +20,8 @@
38
39	import QtQuick 2.0
40	import Ubuntu.Components 0.1
41	-import Unity.Indicators 0.1 as Indicators
42	import Unity.Indicators 0.1
43	+import Utils 0.1 as Utils
44
45	HeroMessage {
46	id: snapDecision
47	@@ -59,7 +59,7 @@
48	height: units.gu(4)
49	opacity: 0.0
50
51	- TimeFormatter {
52	+ Utils.TimeFormatter {
53	id: timeFormatter
54	time: snapDecision.time
55	format: "hh:mm - MMM dd"
56
57	=== modified file 'plugins/Unity/Indicators/plugin.cpp'
58	--- plugins/Unity/Indicators/plugin.cpp 2013-10-16 19:48:57 +0000
59	+++ plugins/Unity/Indicators/plugin.cpp 2013-12-13 13:24:49 +0000
60	@@ -33,7 +33,6 @@
61	#include "unitymenumodelcache.h"
62	#include "unitymenumodelstack.h"
63	#include "visibleindicatorsmodel.h"
64	-#include "timeformatter.h"
65
66	static QObject* menuModelCacheSingleton(QQmlEngine* engine, QJSEngine* scriptEngine) {
67	Q_UNUSED(engine);
68	@@ -52,7 +51,6 @@
69	qmlRegisterType<RootActionState>(uri, 0, 1, "RootActionState");
70	qmlRegisterType<ModelPrinter>(uri, 0, 1, "ModelPrinter");
71	qmlRegisterType<VisibleIndicatorsModel>(uri, 0, 1, "VisibleIndicatorsModel");
72	- qmlRegisterType<TimeFormatter>(uri, 0, 1, "TimeFormatter");
73
74	qmlRegisterSingletonType<UnityMenuModelCache>(uri, 0, 1, "UnityMenuModelCache", menuModelCacheSingleton);
75
76
77	=== modified file 'plugins/Utils/CMakeLists.txt'
78	--- plugins/Utils/CMakeLists.txt 2013-11-22 17:11:03 +0000
79	+++ plugins/Utils/CMakeLists.txt 2013-12-13 13:24:49 +0000
80	@@ -16,6 +16,7 @@
81	mediaartcache.cpp
82	qlimitproxymodelqml.cpp
83	qsortfilterproxymodelqml.cpp
84	+ timeformatter.cpp
85	unitymenumodelpaths.cpp
86	plugin.cpp
87	)
88
89	=== modified file 'plugins/Utils/plugin.cpp'
90	--- plugins/Utils/plugin.cpp 2013-11-22 17:11:03 +0000
91	+++ plugins/Utils/plugin.cpp 2013-12-13 13:24:49 +0000
92	@@ -30,6 +30,7 @@
93	#include "bottombarvisibilitycommunicatorshell.h"
94	#include "qlimitproxymodelqml.h"
95	#include "qsortfilterproxymodelqml.h"
96	+#include "timeformatter.h"
97	#include "unitymenumodelpaths.h"
98
99	static const char* BOTTOM_BAR_VISIBILITY_COMMUNICATOR_DBUS_PATH = "/BottomBarVisibilityCommunicator";
100	@@ -42,6 +43,8 @@
101	qmlRegisterType<QLimitProxyModelQML>(uri, 0, 1, "LimitProxyModel");
102	qmlRegisterType<QSortFilterProxyModelQML>(uri, 0, 1, "SortFilterProxyModel");
103	qmlRegisterType<UnityMenuModelPaths>(uri, 0, 1, "UnityMenuModelPaths");
104	+ qmlRegisterType<TimeFormatter>(uri, 0, 1, "TimeFormatter");
105	+ qmlRegisterType<GDateTimeFormatter>(uri, 0, 1, "GDateTimeFormatter");
106	qmlRegisterUncreatableType<BottomBarVisibilityCommunicatorShell>(uri, 0, 1, "BottomBarVisibilityCommunicatorShell", "Can't create BottomBarVisibilityCommunicatorShell");
107	}
108
109
110	=== renamed file 'plugins/Unity/Indicators/timeformatter.cpp' => 'plugins/Utils/timeformatter.cpp'
111	--- plugins/Unity/Indicators/timeformatter.cpp 2013-10-16 19:48:57 +0000
112	+++ plugins/Utils/timeformatter.cpp 2013-12-13 13:24:49 +0000
113	@@ -167,6 +167,35 @@
114
115	void TimeFormatter::update()
116	{
117	- priv->timeString = QDateTime::fromMSecsSinceEpoch(priv->time / 1000).toString(priv->format);
118	+ priv->timeString = formatTime();
119	Q_EMIT timeStringChanged(priv->timeString);
120	}
121	+
122	+QString TimeFormatter::formatTime() const
123	+{
124	+ return QDateTime::fromMSecsSinceEpoch(time() / 1000).toString(format());
125	+}
126	+
127	+GDateTimeFormatter::GDateTimeFormatter(QObject* parent)
128	+: TimeFormatter(parent)
129	+{
130	+}
131	+
132	+QString GDateTimeFormatter::formatTime() const
133	+{
134	+ gchar* time_string;
135	+ GDateTime* datetime;
136	+ QByteArray formatBytes = format().toUtf8();
137	+
138	+ datetime = g_date_time_new_from_unix_local(time());
139	+ if (!datetime) {
140	+ return "";
141	+ }
142	+
143	+ time_string = g_date_time_format(datetime, formatBytes.constData());
144	+ QString formattedTime(QString::fromUtf8(time_string));
145	+
146	+ g_free(time_string);
147	+ g_date_time_unref(datetime);
148	+ return formattedTime;
149	+}
150
151	=== renamed file 'plugins/Unity/Indicators/timeformatter.h' => 'plugins/Utils/timeformatter.h'
152	--- plugins/Unity/Indicators/timeformatter.h 2013-10-16 19:48:57 +0000
153	+++ plugins/Utils/timeformatter.h 2013-12-13 13:24:49 +0000
154	@@ -19,10 +19,10 @@
155	#ifndef TIME_FORMATTER_H
156	#define TIME_FORMATTER_H
157
158	-#include "unityindicatorsglobal.h"
159	#include <QObject>
160
161	-class UNITYINDICATORS_EXPORT TimeFormatter : public QObject
162	+// TODO - bug #1260728
163	+class TimeFormatter : public QObject
164	{
165	Q_OBJECT
166	Q_PROPERTY(QString format READ format WRITE setFormat NOTIFY formatChanged)
167	@@ -31,7 +31,7 @@
168
169	public:
170	TimeFormatter(QObject *parent = 0);
171	- ~TimeFormatter();
172	+ virtual ~TimeFormatter();
173
174	QString format() const;
175	QString timeString() const;
176	@@ -47,8 +47,20 @@
177	void timeStringChanged(const QString &timeString);
178	void timeChanged(qint64 time);
179
180	+protected:
181	+ virtual QString formatTime() const;
182	+
183	private:
184	struct TimeFormatterPrivate *priv;
185	};
186
187	+class GDateTimeFormatter : public TimeFormatter
188	+{
189	+public:
190	+ GDateTimeFormatter(QObject *parent = 0);
191	+
192	+protected:
193	+ virtual QString formatTime() const;
194	+};
195	+
196	#endif
197
198	=== modified file 'tests/plugins/Unity/Indicators/CMakeLists.txt'
199	--- tests/plugins/Unity/Indicators/CMakeLists.txt 2013-11-06 10:26:58 +0000
200	+++ tests/plugins/Unity/Indicators/CMakeLists.txt 2013-12-13 13:24:49 +0000
201	@@ -40,4 +40,3 @@
202	indicator_test(menucontentactivatortest ADDITIONAL_CPPS ${INDICATORS_DIR}/menucontentactivator.cpp)
203	indicator_test(unitymenumodelstacktest ADDITIONAL_CPPS ${TEST_DIR}/mocks/QMenuModel/unitymenumodel.cpp ${INDICATORS_DIR}/unitymenumodelstack.cpp)
204	indicator_test(rootactionstatetest ADDITIONAL_LIBS IndicatorsQml)
205	-indicator_test(timeformattertest ADDITIONAL_LIBS IndicatorsQml)
206
207	=== modified file 'tests/plugins/Utils/CMakeLists.txt'
208	--- tests/plugins/Utils/CMakeLists.txt 2013-06-05 22:03:08 +0000
209	+++ tests/plugins/Utils/CMakeLists.txt 2013-12-13 13:24:49 +0000
210	@@ -24,4 +24,5 @@
211	run_tests(
212	qlimitproxymodeltest
213	qsortfilterproxymodeltest
214	+ timeformattertest
215	)
216
217	=== renamed file 'tests/plugins/Unity/Indicators/timeformattertest.cpp' => 'tests/plugins/Utils/timeformattertest.cpp'
218	--- tests/plugins/Unity/Indicators/timeformattertest.cpp 2013-10-16 20:17:15 +0000
219	+++ tests/plugins/Utils/timeformattertest.cpp 2013-12-13 13:24:49 +0000
220	@@ -48,6 +48,19 @@
221
222	QCOMPARE(formatter.timeString(), time.toString(format));
223	}
224	+
225	+ void testFormatStrF()
226	+ {
227	+ const QString format = "%d-%m-%Y %I:%M%p";
228	+
229	+ QDateTime time = QDateTime::currentDateTime();
230	+
231	+ GDateTimeFormatter formatter;
232	+ formatter.setTime(time.toMSecsSinceEpoch() / 1000); // strftime in seconds since epoc
233	+ formatter.setFormat(format);
234	+
235	+ QCOMPARE(formatter.timeString(), time.toString("dd-MM-yyyy hh:mmAP"));
236	+ }
237	};
238
239	QTEST_GUILESS_MAIN(TimeFormatterTest)