Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate edit fields #461

Merged
merged 73 commits into from
Oct 30, 2019
Merged

Conversation

ederag
Copy link
Collaborator

@ederag ederag commented Oct 5, 2019

This PR is first step towards verbatim entry.
To fix #280 and #423 some changes will be required in db.py (working on it).

Meanwhile, using completion code/widgets that were still around,
this PR brings back separate field entries,
as well as completion for time, activity, category and tags.
That should fix #301 and #439.

Screenshot_20191005_224618

Feedback welcome !

To do:

  • End time popup is too large
  • Fix empty start time issues
  • Find more standard icons, and/or check icons availability, provide fallback ? cf. Separate edit fields #461 (comment)
  • separate "go-down-symbolic" and clear icons

@ederag
Copy link
Collaborator Author

ederag commented Oct 28, 2019

@CendioOssman

removing the ability to do "activity@category" would be a massive step backwords in usability. It sounds like you're suggesting it will be made to behave exactly the same as the new "Activity" field?

You mean in the cmdline, right ?
In the cmdline, "activity@category" allows to filter on past activities.

My use case is that I have a fixed set of categories, but very often new activities.

Thanks for giving more details on your use case.
Having more new activities than new categories seems natural, you are not alone at all.

With this PR, to enter a new activity, you would use the separate fields,
first category (with completion => no mistake) then TAB to activity and enter the new one.

I do understand that you would like to do that in the cmdline.
That's no small feat.
This PR will allow to simplify the cmdline completion a lot (not started yet).
Maybe then will other completions be brought back.
But that's a pandora box. You are asking for category, others will ask for activity and tags.

Question is, how often do you enter a brand new activity, with respect to reuse an existing activity ?
(what ratio ?)

@ederag
Copy link
Collaborator Author

ederag commented Oct 28, 2019

@mwilck

Given that the separate inputs require between 4 and 8 mouse clicks (or TAB keystrokes) to navigate between the input fields anyway, that doesn't look like a big deterioration to me.

Most of the times these separate fields will be used for one or two changes.
For more extensive changes the cmdline is easier.
So this is not a demonstrative comparison.

Done right, it would be exactly one extra mouse click (the button). The details dialog would be focused/raised automatically, so that no searching would be necessary.

We strongly disagree here.
Locating the entry on the screen
(which is impossible to do before the secondary window or the expander are shown)
takes a non-negligible attention IMO.

Keep these parts of the dialog hidden by default, and simply display expander controls for them.

Expanders need precise clicking, not as user friendly as having the whole entries available.
The following argument also applies here.

Then, please, offer a configuration option to hide these inputs

The arguments given in a previous comment show that this choice will not always be good.
That would increase friction a lot.
There is also a warning about too much configuration in this link, that applies here IMO.

Please, rethink. At least keep the calendar hidden by default, or move it to the bottom.

No need to stress so much on "please".
In general, calm, rational arguments, careful reading of my answers,
and clear explanations of use cases achieve more with me.

I already said that the calendar stuff will be reconsidered later. Not now.

Have you tried the interface now ?
Is it that bad in practice ?

@mwilck
Copy link
Contributor

mwilck commented Oct 28, 2019

@ederag,

Expanders need precise clicking, not as user friendly as having the whole entries available.

This is only true if all entries are necessary, or equally important. Which is IMO not the case, that's my point. Displaying controls that the user doesn't need is confusing and distracting, and likely causes more friction than one extra click.

About "need precise clicking" - I really can't take that argument seriously. GtkExpander widgets can be set to occupy the whole width of the parent widget, making them easier to click than most other widgets in the dialog.

No need to stress so much on "please".

I was trying to be polite. And I was also trying to be rational. I read your answers, too. I just happen not to be convinced, and was trying to point out why. Apparently you got a different impression. I'm sorry about that. I really appreciate your work for hamster, no mistakes.

Have you tried the interface now ?
Is it that bad in practice ?

I'm not saying it's "bad". I say it's too much, too big, distracting.

Here's one benefit I see: the different autocompletion in the "activity" field, with it's "filtered by category" option. That's a nice complement to the autocompletion of the free text field, which only shows recent activities. I'm not sure whether the "autocompletion mode" of the free-entry field couldn't be likewise improved, though.

I find the behavior of the calendar widget irritating. My expectation was that clicking on a different date than today would add a date in the "cmdline" field. That's not the case - clicking on the date only changes the "dayline" widget at the top. I can see that the reason for this is that date setting / parsing from the cmdline in the "edit/add activity" widget never worked as it should. Yet with an explicit date widget, I expected this to be fixed. In general, it's a bit confusing that some of the controls are linked with the command line field, while others are not.

Actually, call me dumb, but the dialog gave me no explicit clue what effect the date widgets might have. Yet when I clicked "save", the date was actually taken into account for the created/modified entry. I guessed so, but it wasn't at all clear to me as a "novice" user of this dialog. Admitted, it's no worse than in v2.2, but it isn't a lot better either (and unlike 2.2, the dialog now kind of encourages me to play with the date widgets).

I believe that this is a bit dangerous. It's easy to accidentally click on the calendar somewhere without noticing (did I mention it occupies a lot of space?), and then wonder why the item isn't showing in the overview (because it ended up being saved for a different day).

I hope you find this rational and clear. I'm trying my best.

@ederag
Copy link
Collaborator Author

ederag commented Oct 28, 2019

@mwilck Thanks for the feedback.

Displaying controls that the user doesn't need is confusing and distracting, and likely causes more friction than one extra click.

Fully agreed in general. I did read this nice article about the number of clicks earlier.
In the present case though, after using it for a few weeks,
when I enter from keyboard in the cmdline, I hardly even look at the other entries.
Of course it might be a developer bias,
and please believe that I'm not taking your argument lightly. This is indeed worrying.

GtkExpander widgets can be set to occupy the whole width of the parent widget, making them easier to click than most other widgets in the dialog.

Interesting, I had another type in mind, with a small arrow to reveal/hide. Maybe not a Gtk one.

I read your answers, too. I just happen not to be convinced, and was trying to point out why.

Thanks for that, well explained feedback is precious.

That's a nice complement to the autocompletion of the free text field, which only shows recent activities.

Good, thanks for confirmation.

I'm not sure whether the "autocompletion mode" of the free-entry field couldn't be likewise improved, though.

I already said that opening this pandora box should be reconsidered later.
Beware that wanting everything in v3.0 is a good way to never see that release.

I find the behavior of the calendar widget irritating. My expectation was that clicking on a different date than today would add a date in the "cmdline" field. That's not the case - clicking on the date only changes the "dayline" widget at the top. I can see that the reason for this is that date setting / parsing from the cmdline in the "edit/add activity" widget never worked as it should. Yet with an explicit date widget, I expected this to be fixed.

Indeed, that's related to #438.
I need a break. Has it really to be solved for v3.0 ?

The initial plan was to show date in cmdline only if it did not match the day set in timeline.
(that is, only for facts overlapping day-start)
Otherwise the cmdline will be cluttered with the start and end dates.

Actually, call me dumb, but the dialog gave me no explicit clue what effect the date widgets might have. Yet when I clicked "save", the date was actually taken into account for the created/modified entry. I guessed so, but it wasn't at all clear to me as a "novice" user of this dialog. Admitted, it's no worse than in v2.2, but it isn't a lot better either (and unlike 2.2, the dialog now kind of encourages me to play with the date widgets).

Could it be partly because you decided that the calendars were useless,
and did not want to see them here in the first place ?
They are "what you see is what will get into the database".

I believe that this is a bit dangerous. It's easy to accidentally click on the calendar somewhere without noticing (did I mention it occupies a lot of space?), and then wonder why the item isn't showing in the overview (because it ended up being saved for a different day).

That accidental click is another valid argument.
It is true that changing dates is dangerous,
and that it is more prone to happen with calendars than with the timeline arrows.
The best fix for this danger would be to highlight the potential overlapping facts
in the timeline. Or at least to display a warning on the save button.
That's also in the long term plans.
Finding the overlapping facts has to be separated from the __solve_overlaps first.

Need to rest and think.

I hope you find this rational and clear. I'm trying my best.

Both rational and clear. Overall, very useful feedback, thanks !

@mwilck
Copy link
Contributor

mwilck commented Oct 29, 2019

@ederag, I fully understand that you need a break and that you want to get this off the table.
So please let my nitpicking here not stop you. I feel we agree on most issues.

Yet there's one thing that occured to me about how this newly designed dialog might influence user expectations, that I'd like to ask you to think about.

In theory, the separate entry fields would allow to overcome certain restrictions on input format, such as #280. And this we should not do.

To elaborate: The current dialog allows entering "x,y" in the activity field. If "saved" is clicked, this is transformed into an actitivity "x" with description "y". This will irritate users. Yet, it is better than forcing the "x,y" activity name to be set in the hamster data base, because that would break the equivalence between the cmdline and multi-field input. An activity set this way couldn't ever be edited or re-entered using the cmdline (or worse, trying to do that would probably generate a different, new entry).

Thinking about it, more than any nitpicking about calendar widget layout, this is my main concern - the new dialog would encourage users to write a bug report that entering "x,y" in the activity field doesn't do what they expect, and such a bug might be quickly fixed without much thinking, breaking the command line on the way. This, I fear, opens up a Pandora's Box of user expectations.

For me, being able to fully control hamster with the single command line is absolutely a key feature of hamster which we shouldn't even think about changing.

Therefore I suggest that you minimal syntax checking be added to the activity and category fields, and indicate an error (e.g. by changing the bg color to red) when e.g. a comma is entered in the activity field.

I wouldn't consider even that a show stopper, but IMO it should be added quickly after a 3.0 release, to avoid user expectations to grow into the wrong direction.

@mwilck
Copy link
Contributor

mwilck commented Oct 29, 2019

Here's one more nit - try entering strings with spaces (or just tags separated by space not comma, as users might attempt) in the "tags" field, it has unexpected effects. If there's a nonempty description, the tag string will be appended to it. If there is no description, the tag string is appended to the category name, and if the category isn't filled in either, the tag string will become the category.

Well actually here, by observing carefully what happens in the cmdline field when I do this, I could foresee that this wasn't going to work as I expected. Yet I believe it will cause confusion. Syntax checking and not accepting invalid fields might be in order here, too.

@ederag
Copy link
Collaborator Author

ederag commented Oct 29, 2019

@mwilck Justified concerns.

This PR is part of a larger plan.
When it is finished, there will be a much better separation of concerns,
as well as more possibilities,
while keeping existing usages safe.

That's why the header said

To fix #280 and #423 some changes will be required in db.py (working on it).

Actually it's done and just waiting for this PR.

being able to fully control hamster with the single command line is absolutely a key feature

If you accept to stick to current limitations, this will still be possible, no change.

Yet parser limitations have nothing to do with gui (#280 (comment)).

Maybe there is a purpose for that in CLI version but in GUI it is confusing.

More importantly from a developer point of view:
The current stage makes any parser change scary (and some changes might not be deliberate).
After a few changes, old facts in the database can be impossible to edit or even see (#423).
Legacy code is really good, but that part should be improved IMO.

The plan is to make database and the separate fields "verbatim", without any parsing.
Parsing is confined to command line and gui cmdline.
(this is ready to be pushed)

What would be important to have is an indicator
about whether the fact entered through separate fields is parseable or not.
(probably through Fact.parse(repr(fact)) == fact).

What should be this indicator ?
Please open an issue after this one is merged, to discuss that.

Hiding the calendar is almost finished. Stay tuned.

@ederag
Copy link
Collaborator Author

ederag commented Oct 29, 2019

Done, calendar is hidden by default,
but date is shown, including week-day.
This should hopefully prevent most mistakes.
Screenshot_20191029_134925

The label fill is on, yet the label itself has to be clicked,
the clickable area does not extend to the full width.
That's why I left the date label also when the calendar is shown.

@ederag
Copy link
Collaborator Author

ederag commented Oct 29, 2019

Forgot to push the label fill stuff.
@mwilck

GtkExpander widgets can be set to occupy the whole width of the parent widget

Did nothing here. Is it my system (gtk-3.22) ?

That can be fixed later. Let's merge this one tomorrow.

@ederag ederag merged commit 1e1b2ad into projecthamster:master Oct 30, 2019
@ederag ederag deleted the separate_edit_fields branch October 30, 2019 12:44
@GeraldJansen
Copy link
Contributor

Hurray!

@ederag
Copy link
Collaborator Author

ederag commented Oct 30, 2019

Merged, thanks for the feedback !

Reminders:

@mwilck
Copy link
Contributor

mwilck commented Nov 4, 2019

@ederag, many thanks for your late improvements.

Database access should be verbatim to fully exploit the separate fields

I'm not happy about this part of the update, rationale. I believe that adding the ability to enter content to the data base that can't be parsed was a move in the wrong direction. The intention to improve the parser (#465) is good, but maybe we should first have discussed what the parser could possibly do. The current rules (no quoting, and spaces allowed in tokens) make it essentially impossible to write a parser that could be fully compatible to separate fields input.

Anyway, it's how it is now. Let's hope that users don't shoot themselves in their feet with the separate entry fields. I for one am not going to use them anyway.

@ederag
Copy link
Collaborator Author

ederag commented Nov 4, 2019

@mwilck Changing the parser is no small feat,
and do not fix several bug that will need separate fields anyway.
Your concern has already been taken seriously, and is tracked in #467.

@GeraldJansen
Copy link
Contributor

Separate input fields seems to be important to some users and will ease the transition from v1, so fine. I may even use them occasionally.

What I really dislike is the whole idea of verbatim input fields allowing input that cannot be edited in the cmdline (or input from the CLI), and then having to visually signal non-standard inputs. That seems pretty messy to me. Releasing this and PR 466 will allow input like this:

./hamster-cli list
Start | End | Duration | Activity                               | Category
-----------------------------------------------------------------------------
10:51 |     | 0h 46min | x@y  bis #123, some text #tag@Unsorted | zzz     
-----------------------------------------------------------------------------

Once these changes are released into the wild, it will be too late to start discussing future parsing improvements.

I suggest a more cautious approach for the upcoming release. In the activity entry field, only allow '#' as the very first character (as per the original request), and disallow ',' and '@'. This would require a minimal change to the cmdline parser to allow the initial '#'.

@mwilck
Copy link
Contributor

mwilck commented Nov 4, 2019

@GeraldJansen, full ACK.

@CendioOssman
Copy link
Contributor

A bit late to the party, but for future reference. :)

Question is, how often do you enter a brand new activity, with respect to reuse an existing activity ?
(what ratio ?)

The ratio would be close to infinite in favour of existing categories. In fact, for me it would be a feature if new categories could not be entered that way and have to be explicitly set up.

@mwilck
Copy link
Contributor

mwilck commented Nov 13, 2019

The ratio would be close to infinite in favour of existing categories. In fact, for me it would be a feature if new categories could not be entered that way and have to be explicitly set up.

Good point. This holds for most long-time users I guess. But "explicitly set up" should not mean having to open the "Tracking Settings/Categories and Tags" dialog. That would be way too explicit for my taste.

@ederag ederag mentioned this pull request Nov 16, 2019
@ederag
Copy link
Collaborator Author

ederag commented Nov 16, 2019

@CendioOssman

The ratio would be close to infinite in favour of existing categories

Thanks for the confirmation.

@GeraldJansen, @mwilck Granted, #465 has not been demontrated yet,
so taking into account your concerns,
facts are not allowed to be saved from the gui if they can not be parsed back (PR #476).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Design discussions and interesting use cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No completion for category (regression) Comma in activity moves to description
5 participants