[#136] Add 'recent activities' to tracking screen #208

elbenfreund · 2017-07-08T12:10:14Z

This adds a 'recent activities' area to the tracking screen that can be used to take previous activities as templates for new raw fact strings or start tracking them right away. The entire functionality is optional (on by default right now) and can be configures via preferences.

The 'recent activities' box size can be adjusted by specifying the amount of activities it shall be able to show. All activities that not fit are available via scrolling.

Closes: #136

jtojnar · 2017-09-15T15:27:49Z

hamster_gtk/tracking/screens.py

+    def _on_config_changed(self, sender):
+        """Callback triggered when 'config-changed' event fired."""
+        if self._app.config['tracking_show_recent_activities']:
+            # We re-create it even if one existed before necause its parameters


jtojnar · 2017-09-15T15:31:26Z

hamster_gtk/preferences/preferences_dialog.py

@@ -80,6 +80,11 @@ def __init__(self, parent, app, initial, *args, **kwargs):
                ('autocomplete_split_activity',
                 (_("Autocomplete activities and categories separately"),
                  HamsterSwitch())),
+                ('tracking_show_recent_activities',
+                 (_("Allow tracking based on recent activities."),


This does not sound clear to me. I would choose something like “Show recent activities for quickly starting tracking”

jtojnar · 2017-09-15T15:32:31Z

hamster_gtk/helpers.py

+    return OrderedSet(recent_activities)
+
+
+def serialize_activity(activity):


Do not we have something like this somewhere already?

I think earlier hamster-lib branches had something like this. Also the overview has its own "serialize activity" code which is different however:

it deals with legacy hamerest representation of activity.category=None. This is something that I would be willing to throw out of the window.

It uses - as seperator between activity.name and category. I think this is better looking for the overview. What do you think?

So this helper provides a generic serialization utilizing the standart @ seperator. As far as I can tell current versions of hamster-lib or hamster-gtk do not have such a function yet. But share the feeling that we had this around for a while in various incarnations...

Wait, do we intend to not support category-less facts?

I actually prefer the at sign for consistency or habit.

No we do and want to support category less facts. I am not quite sure that the way legacy hamster did is really where we want to go..

Does that mean you would prefer @ in the overview as well? At this stage I do not care about this detail very much as we will have to revisit the general layout at some later stage anyway...

Yes, having @ in overview is what I am accustomed to.

Alrighty. Will adjust this then :) Im cool either way tbh.

jtojnar · 2017-09-15T15:33:50Z

hamster_gtk/hamster_gtk.py

@@ -279,6 +285,8 @@ def _get_default_config(self):
            # Frontend
            'autocomplete_activities_range': 30,
            'autocomplete_split_activity': False,
+            'tracking_show_recent_activities': True,
+            'tracking_recent_activities_items': 6,


I would use count or number. items is less clear.

jtojnar · 2017-09-15T15:54:36Z

hamster_gtk/tracking/screens.py

+        # allocated to its children so they actually have a height that we can
+        # use.
+        grid.show_all()
+        # We fetch an arbitrary Button as height-reference


this is really ugly and crashes when there are no items:

$ env XDG_CONFIG_HOME=$HOME/.dev_config XDG_DATA_HOME=$HOME/.dev_local hamster-gtk Hamster-GTK started. Traceback (most recent call last): File "/nix/store/fzrxvdjas3icqb2n5akahvix758jnfvj-python3.6-hamster-gtk/lib/python3.6/site-packages/hamster_gtk/hamster_gtk.py", line 213, in _activate self.window = MainWindow(app) File "/nix/store/fzrxvdjas3icqb2n5akahvix758jnfvj-python3.6-hamster-gtk/lib/python3.6/site-packages/hamster_gtk/hamster_gtk.py", line 107, in __init__ self.add(TrackingScreen(self.app)) File "/nix/store/fzrxvdjas3icqb2n5akahvix758jnfvj-python3.6-hamster-gtk/lib/python3.6/site-packages/hamster_gtk/tracking/screens.py", line 49, in __init__ self.start_tracking_view = StartTrackingBox(self._app) File "/nix/store/fzrxvdjas3icqb2n5akahvix758jnfvj-python3.6-hamster-gtk/lib/python3.6/site-packages/hamster_gtk/tracking/screens.py", line 195, in __init__ self.recent_activities_widget = self._get_recent_activities_widget() File "/nix/store/fzrxvdjas3icqb2n5akahvix758jnfvj-python3.6-hamster-gtk/lib/python3.6/site-packages/hamster_gtk/tracking/screens.py", line 255, in _get_recent_activities_widget child = grid.get_children()[1] IndexError: list index out of range Traceback (most recent call last): File "/nix/store/fzrxvdjas3icqb2n5akahvix758jnfvj-python3.6-hamster-gtk/lib/python3.6/site-packages/hamster_gtk/hamster_gtk.py", line 218, in _activate app.add_window(self.window) TypeError: Argument 1 does not allow None as a value Hamster-GTK shut down.

I agree that the way we derive our min_height hint is far from elegant (but fixed value layouts are far worse even). I am absolutely open to better approaches! Empty children are definitely a nice catch and have been addressed in the latest version.

Maybe add a todo comment and go with this.

I created #224 as I would like to avoid todo comments.

jtojnar · 2017-09-15T15:57:56Z

hamster_gtk/tracking/screens.py

+
+    def _populate(self):
+        """Fill the widget with rows per activity."""
+        def add_row_widgets(row_counter, activity):


row_counter → row_index/row_number; counter implies an object that counts

jtojnar · 2017-09-15T16:19:49Z

hamster_gtk/helpers.py

+def get_recent_activities(controller, start, end):
+    """Return a list of all activities logged in facts within the given timeframe."""
+    recent_activities = [fact.activity for fact in controller.facts.get_all(start=start, end=end)]
+    return OrderedSet(recent_activities)


The activities are returned in a convoluted order. It should be reversed prior to removing duplicates.

I am not sure I do understand. Could you elaborate please?

It should be OrderedSet(reversed(recent_activities)), otherwise you will get the activities in the order they were first used, not the last.

facts 1 2 1 3 2 1 3 output reversed current x x x → 1 2 3 → 3 2 1 expected x x x → 3 1 2 → 2 1 3

Thank you for the clarification. To be honest, right now FactManager.get_all() does not commit to any ordering at all. I created an issue with hamster-lib to improve this.
For now I will implement a workaround simple sorting call as part of the prelimery get_recent_activities helper which will hopefully address your concerns.

jtojnar · 2017-09-15T16:22:04Z

hamster_gtk/tracking/screens.py

+
+        today = datetime.date.today()
+        start = today - datetime.timedelta(90)
+        activities = reversed(helpers.get_recent_activities(self._controller, start, today))


Do we really want all the activities and scroll instead of displaying the tracking_recent_activities_items?

I would prefer such a more general solution. There is nothing preventing us from limiting the results to just tracking_recent_activities_items (via config) if we want, but especially given that we already have a solution I do not see the harm.

We need scrolling anyway (because we can not make any guarantee that tracking_recent_activities_items will fit in the space available. In light of that, it seems we get the extra activities for free if the user bothers scolling.

What would be the downside?

Accidental scrolling to the other end and performance.

"accidental scrolling": Im not sure I see the use case. especially as there is the posibility of a scrollbox even if if we just fetch the number of activities defined in the config.

by default we just fetch todays facts. this should net us no more than a couple of dozens facts at most I figure. I doubt there is any concievable performance impact on the DB or the GUI rendering. Unless I'm missing something this seems like a case of premature optimization.

You scroll somewhere accidentally and since the list is long you do not have a simple way of returning back – you cannot just quickly scroll down and when you find out that you are on the wrong side quickly scroll to the correct one.

I see more than just today’s facts. In fact there does not seem to be any limit.

You are right!

The idea was to make the timedelta adjustable via a config setting, but i forgot to actually do that :(
If done that way it is up to the user to figure what timeframe is viable to them. We probably should point out the performance consideration though...

It would probably be best to leave this additional customization to a seperate issue and use start=today for now...

Closes: 206

This commits extends the 'start tracking' view with a list of recent activities that can be used to euther copy the data to the entry field, ready to be extended and then started manually by the user or to start/continue tracking this activity right away. Ref: #136

codecov · 2017-09-22T12:37:03Z

Codecov Report

Merging #208 into develop will decrease coverage by 2.37%.
The diff coverage is 55.76%.

@@             Coverage Diff             @@
##           develop     #208      +/-   ##
===========================================
- Coverage    82.59%   80.21%   -2.38%     
===========================================
  Files           31       31              
  Lines         1356     1456     +100     
  Branches        78       86       +8     
===========================================
+ Hits          1120     1168      +48     
- Misses         213      260      +47     
- Partials        23       28       +5

Impacted Files	Coverage Δ
hamster_gtk/preferences/preferences_dialog.py	`100% <ø> (ø)`	⬆️
hamster_gtk/overview/widgets/fact_grid.py	`83.2% <100%> (+1.31%)`	⬆️
hamster_gtk/tracking/screens.py	`69.58% <46.66%> (-14.45%)`	⬇️
hamster_gtk/helpers.py	`80.28% <68.75%> (-3.36%)`	⬇️
hamster_gtk/hamster_gtk.py	`82.64% <90.9%> (-3.41%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b5d106...1df269a. Read the comment docs.

jtojnar · 2017-09-22T12:37:59Z

hamster_gtk/tracking/screens.py

@@ -45,7 +45,8 @@ def __init__(self, app, *args, **kwargs):
        self.set_transition_type(Gtk.StackTransitionType.SLIDE_UP)
        self.set_transition_duration(1000)
        self.current_fact_view = CurrentFactBox(self._app.controller)
-        self.current_fact_view.connect('tracking-stopped', self.update)
+        self.current_fact_view.con275G


Until now, we returned a ``PathLocal`` instance as value for the ``tmpfile_path`` key. This has been changed to a proper string representations instead.

Our ``app`` fixture (which is used extensively) used to instantiate the default ``HamsterGTK`` class. This meant that the default config loading was triggered and as a consequence the systems live config being used. This is obviously an issue. Instead we monkey patch the class now to skip the config loading machinery and use/return the ``config`` fixture.

Our previous test simply instantiated the class, hence tiggering the config loading/creation machinery. This will directly interfere with the actual user config! To avoid this we provide a new test that monkeypatches the class accordingly. As a consequence the config loading needs to be tested seperatly now though.

elbenfreund · 2017-11-17T13:39:12Z

@jtojnar I addressed most of your concerns and CI is passing again (finally). Do you think you will find some time to review it again?

The only big issue left open is this. I am absolutely open to improve the current hackish solution if you have better idea.

jtojnar

Several typos, otherwise looks good from a shallow review (ignoring my bloat concerns)

jtojnar · 2017-09-27T10:14:15Z

tests/test_hamster-gtk.py

-        app = hamster_gtk.HamsterGTK()
+    def test_instantiation(self, config):
+        """
+        Make sure class instatiation works as intended.


instatiation → instantiation

jtojnar · 2017-11-17T15:50:00Z

hamster_gtk/helpers.py

+        separator (str, optional): ``string`` used to separate ``activity.name`` and
+            ``category.name``. The separator will be omitted if ``none_categoty=''`` and
+            ``activity.category=None``. Defaults to ``@``.
+        none_category (str, optional): ``string`` to represent the 'lack of a cactegory' for an


cactegory

jtojnar · 2018-05-04T11:41:09Z

hamster_gtk/helpers.py

+def get_recent_activities(controller, start, end):
+    """Return a list of all activities logged in facts within the given timeframe."""
+    # [FIXME]
+    # This manual sorting within python is of cause less than optimal. We stick


of cause → of course

jtojnar · 2018-05-04T11:41:49Z

hamster_gtk/helpers.py

+    Args:
+        activity (Activity): ``Activity`` instance to serialize.
+        separator (str, optional): ``string`` used to separate ``activity.name`` and
+            ``category.name``. The separator will be omitted if ``none_categoty=''`` and


none_categoty → none_category

jtojnar · 2018-05-04T11:44:59Z

tests/test_hamster-gtk.py

+
+        We actually test against a monkeypatched class in order to avoid the
+        config loading machinery as this would access the user data on fs.
+        The relevant skiped methods need to be tested separately.


jtojnar · 2018-05-04T11:46:28Z

hamster_gtk/tracking/screens.py

+        # allocated to its children so they actually have a height that we can
+        # use.
+        grid.show_all()
+        # We fetch an arbitrary Button as height-reference


Maybe add a todo comment and go with this.

jtojnar · 2018-05-04T12:42:42Z

hamster_gtk/helpers.py

+    return OrderedSet(recent_activities)
+
+
+def serialize_activity(activity, separator='@', none_category='not categorized'):


We probably do not want to serialize uncategorized activities with the “not categorized” category.

If you copy/start them, the fake category will become real.

good catch. 👍

Ref.: #136

When serializing facts for the 'continue tracking' functionality, we need to make sure that the 'uncategorized' placeholder is omitted, otherwise a category of that name would be created! Ref.: #136

Ref,: #136

hamster_gtk/tracking/screens.py

+            """
+            def get_label(activity):
+                """Label representing the activity/category combination."""
+                label = Gtk.Label(helpers.serialize_activity(activity, none_category=''))


elbenfreund added this to the 0.12.0 milestone Jul 8, 2017

elbenfreund requested a review from jtojnar July 8, 2017 12:10

jtojnar requested changes Sep 15, 2017

View reviewed changes

elbenfreund added 4 commits September 15, 2017 20:37

Wrap 'make test' with 'xvfb-run'

54fcc75

Closes: 206

Add inline comments

975623e

Add dedicated helper for fetching recent activities

334753c

elbenfreund force-pushed the feature/136 branch from b41cbed to c17c7d4 Compare September 15, 2017 18:47

elbenfreund added 2 commits September 15, 2017 20:49

Sizing for recent activity widget

5c17e7a

Adjust existing tests

d941aab

elbenfreund force-pushed the feature/136 branch from c17c7d4 to d941aab Compare September 15, 2017 18:49

jtojnar reviewed Sep 22, 2017

View reviewed changes

elbenfreund force-pushed the feature/136 branch from b98097c to 63762a9 Compare September 22, 2017 12:41

elbenfreund added 3 commits September 22, 2017 15:58

rename 'row_counter' parameter to 'row_index'

8593902

Fix typo

c08bd74

Change wording within preferences.

bec62d8

elbenfreund force-pushed the feature/136 branch from aa0d45c to bec62d8 Compare September 22, 2017 13:58

fix ordering for 'get_recent_activities' helper

ddafbd1

elbenfreund force-pushed the feature/136 branch from cd3947c to ddafbd1 Compare September 22, 2017 15:46

Account for empty children when figuring out min_height

fa93742

elbenfreund mentioned this pull request Sep 26, 2017

Make the timeframe for fetching 'rececent_actities' for the tracking box configurable #217

Open

elbenfreund added 5 commits September 26, 2017 16:04

Recent activities box uses last 24h

5b16ae9

Fix docstring

bd39965

Improve 'config' fixture

9ccdc80

Until now, we returned a ``PathLocal`` instance as value for the ``tmpfile_path`` key. This has been changed to a proper string representations instead.

elbenfreund force-pushed the feature/136 branch from 3fe166f to edbf341 Compare November 17, 2017 13:20

jtojnar approved these changes May 4, 2018

View reviewed changes

jtojnar reviewed May 4, 2018

View reviewed changes

Rename 'tracking_recent_activities_items' to 'count'

c0dd639

elbenfreund force-pushed the feature/136 branch from 8f98838 to 5e24b5e Compare May 4, 2018 13:31

elbenfreund added 3 commits May 4, 2018 15:55

helpers: Refactor 'serialize_activity'

2177d68

Ref.: #136

tracking: Improve categoryless fact serialization

4b2819b

When serializing facts for the 'continue tracking' functionality, we need to make sure that the 'uncategorized' placeholder is omitted, otherwise a category of that name would be created! Ref.: #136

Fix typos

90dbcc7

elbenfreund force-pushed the feature/136 branch from 2600c6f to 90dbcc7 Compare May 4, 2018 13:56

tracking: Add issue reference to ToDo comment

fa8057d

Ref,: #136

jtojnar reviewed May 4, 2018

View reviewed changes

hamster_gtk/tracking/screens.py Outdated

"""

def get_label(activity):

"""Label representing the activity/category combination."""

label = Gtk.Label(helpers.serialize_activity(activity, none_category=''))

This comment was marked as resolved.

Sign in to view

elbenfreund force-pushed the feature/136 branch 5 times, most recently from a2f4df4 to a33d84c Compare May 4, 2018 15:07

elbenfreund added 4 commits May 6, 2018 14:05

helpers: Remove 'none_category' argument on serialize_activity

72af08c

tests: Fix docker setup

d685bf2

Fix imports

c510594

helpers: Add tests for 'serialize_activity'

1df269a

elbenfreund force-pushed the feature/136 branch from a33d84c to 1df269a Compare May 6, 2018 12:06

		return OrderedSet(recent_activities)


		def serialize_activity(activity):

		return OrderedSet(recent_activities)


		def serialize_activity(activity, separator='@', none_category='not categorized'):

[#136] Add 'recent activities' to tracking screen #208

Are you sure you want to change the base?

[#136] Add 'recent activities' to tracking screen #208

Conversation

elbenfreund commented Jul 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elbenfreund Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elbenfreund Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elbenfreund Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elbenfreund Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Sep 22, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

elbenfreund commented Nov 17, 2017

jtojnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

elbenfreund commented Jul 8, 2017 •

edited

Loading

elbenfreund Sep 22, 2017 •

edited

Loading

elbenfreund Sep 22, 2017 •

edited

Loading

elbenfreund Sep 22, 2017 •

edited

Loading

elbenfreund Sep 22, 2017 •

edited

Loading

codecov bot commented Sep 22, 2017 •

edited

Loading