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

Logs #164

Merged
merged 56 commits into from Dec 4, 2017

Conversation

Projects
None yet
6 participants
@UziTech
Contributor

UziTech commented Jul 24, 2017

Description of the Change

Add a notification log.

I added a working version of the sm-logs branch.

Todo:

  • Show notifications in log
  • Command to toggle log
  • Use dock instead of panel
  • Click to expand notification #29 (comment)
  • Sort by/Hide categories
  • Show buttons?
  • Counter on status bar #29 (comment)
  • Disable popups setting (in another pull request)
  • Timestamps

Alternate Designs

As I've been writing this I've realized this might be better as an additional package.

The way notifications are re-shown would require this to be integrated.

I tried initially just expanding the notification in the log instead of showing it as a pop up but the logic to render fatal errors would need to be reused. It was easier to re-open the pop up notification.

Benefits

See past notifications.

Possible Drawbacks

none

Applicable Issues

#29

@UziTech UziTech referenced this pull request Jul 24, 2017

Closed

Add notification log viewer panel #29

2 of 4 tasks complete
@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Jul 25, 2017

Contributor

This also seems to fix #110 so we wouldn't need pull request #163

I re-show the notifications like @simurai showed in #29 (comment)

screenshot

Contributor

UziTech commented Jul 25, 2017

This also seems to fix #110 so we wouldn't need pull request #163

I re-show the notifications like @simurai showed in #29 (comment)

screenshot

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jul 26, 2017

Member

Excited for this.

Member

50Wliu commented Jul 26, 2017

Excited for this.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jul 26, 2017

Member

Hmm...maybe I broke it? Looks like the notification got cut off (editor:log-cursor-scopes).
cut-off-notification

Member

50Wliu commented Jul 26, 2017

Hmm...maybe I broke it? Looks like the notification got cut off (editor:log-cursor-scopes).
cut-off-notification

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Jul 26, 2017

Contributor

nice catch. looks like I will have to figure out what to do with html in the message

Contributor

UziTech commented Jul 26, 2017

nice catch. looks like I will have to figure out what to do with html in the message

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Jul 26, 2017

Contributor

I would like some feedback on a few design decisions:

  1. Should the most recent notification be at the top or bottom?

    I'm leaning toward the top since the most common use of the log is to look at a recent notification, but that kind of seems unintuitive.

  2. Should I show the whole message in the case of a multi-line message or limit it to one line?

    I think one line would work as a summary since they can open the notification to see more.

  3. Should I show buttons in the log?

    My only concern is that fatal buttons might change after the issue is checked. I'm not sure how to get the updated button.

    Also if the log is in the left or right dock there isn't much room for the buttons.

  4. Should I include a timestamp with the notifications in the log? maybe as a tooltip?

/cc @50Wliu @simurai

Contributor

UziTech commented Jul 26, 2017

I would like some feedback on a few design decisions:

  1. Should the most recent notification be at the top or bottom?

    I'm leaning toward the top since the most common use of the log is to look at a recent notification, but that kind of seems unintuitive.

  2. Should I show the whole message in the case of a multi-line message or limit it to one line?

    I think one line would work as a summary since they can open the notification to see more.

  3. Should I show buttons in the log?

    My only concern is that fatal buttons might change after the issue is checked. I'm not sure how to get the updated button.

    Also if the log is in the left or right dock there isn't much room for the buttons.

  4. Should I include a timestamp with the notifications in the log? maybe as a tooltip?

/cc @50Wliu @simurai

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jul 26, 2017

Member

Should the most recent notification be at the top or bottom?

I'm also leaning towards the top.

Should I show the whole message in the case of a multi-line message or limit it to one line?

Summary sounds reasonable.

Should I show buttons in the log?

This would be 🆒 if the button updating thing can be worked out.

Should I include a timestamp with the notifications in the log? maybe as a tooltip?

Also cool. If there's enough space maybe it can go on the far right.

Member

50Wliu commented Jul 26, 2017

Should the most recent notification be at the top or bottom?

I'm also leaning towards the top.

Should I show the whole message in the case of a multi-line message or limit it to one line?

Summary sounds reasonable.

Should I show buttons in the log?

This would be 🆒 if the button updating thing can be worked out.

Should I include a timestamp with the notifications in the log? maybe as a tooltip?

Also cool. If there's enough space maybe it can go on the far right.

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Jul 27, 2017

Contributor

I think this is done (except for tests) I'd like to do the status bar counter and disable popups setting in a separate pull request

Contributor

UziTech commented Jul 27, 2017

I think this is done (except for tests) I'd like to do the status bar counter and disable popups setting in a separate pull request

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jul 27, 2017

Member

What do you think about using something like moment.js for handling the relative time?

Member

50Wliu commented Jul 27, 2017

What do you think about using something like moment.js for handling the relative time?

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Jul 27, 2017

Contributor

That could work. I've never dealt with relative time before.

Contributor

UziTech commented Jul 27, 2017

That could work. I've never dealt with relative time before.

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Jul 27, 2017

Contributor

I noticed in a couple other languages that the timestamp will overflow the box when it is under a minute.

I'm not sure if the best solution is to just let it over flow so they can see part of it, or expand the box so it isn't aligned with the other timestamps.

image

image

Contributor

UziTech commented Jul 27, 2017

I noticed in a couple other languages that the timestamp will overflow the box when it is under a minute.

I'm not sure if the best solution is to just let it over flow so they can see part of it, or expand the box so it isn't aligned with the other timestamps.

image

image

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Jul 28, 2017

Contributor

I think I solved it by removing the border on the left and re-working how the buttons and timestamp are laid out

Contributor

UziTech commented Jul 28, 2017

I think I solved it by removing the border on the left and re-working how the buttons and timestamp are laid out

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Aug 1, 2017

Member

@UziTech Should the most recent notification be at the top or bottom?
@50Wliu I'm also leaning towards the top.

Another vote for top.

@UziTech Should I show the whole message in the case of a multi-line message or limit it to one line?
@50Wliu Summary sounds reasonable.

Yeah, one line should be fine and make it easier to scan. Maybe, it could be expanded as an alternative to open a "popup" notification, but out of scope for this PR.

@UziTech Should I show buttons in the log?
@50Wliu This would be 🆒 if the button updating thing can be worked out.

Should the buttons be left aligned, meaning they would show up right after the text:

Before:
image

After:
image

They won't align, since everything has a dynamic width, but clashes less with the "time ago". Or table columns?

@UziTech Should I include a timestamp with the notifications in the log? maybe as a tooltip?
@50Wliu Also cool. If there's enough space maybe it can go on the far right.

👍

Member

simurai commented Aug 1, 2017

@UziTech Should the most recent notification be at the top or bottom?
@50Wliu I'm also leaning towards the top.

Another vote for top.

@UziTech Should I show the whole message in the case of a multi-line message or limit it to one line?
@50Wliu Summary sounds reasonable.

Yeah, one line should be fine and make it easier to scan. Maybe, it could be expanded as an alternative to open a "popup" notification, but out of scope for this PR.

@UziTech Should I show buttons in the log?
@50Wliu This would be 🆒 if the button updating thing can be worked out.

Should the buttons be left aligned, meaning they would show up right after the text:

Before:
image

After:
image

They won't align, since everything has a dynamic width, but clashes less with the "time ago". Or table columns?

@UziTech Should I include a timestamp with the notifications in the log? maybe as a tooltip?
@50Wliu Also cool. If there's enough space maybe it can go on the far right.

👍

@simurai simurai referenced this pull request Aug 1, 2017

Merged

Logs styling #1

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Aug 2, 2017

Contributor

@simurai it looks like the transparent top border is making the icon border perforated

image

Contributor

UziTech commented Aug 2, 2017

@simurai it looks like the transparent top border is making the icon border perforated

image

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Aug 2, 2017

Member

@UziTech it looks like the transparent top border is making the icon border perforated

👍 Right. Ok, switched to box-sizing: content-box; instead of the border.

image

Member

simurai commented Aug 2, 2017

@UziTech it looks like the transparent top border is making the icon border perforated

👍 Right. Ok, switched to box-sizing: content-box; instead of the border.

image

@UziTech UziTech changed the title from [wip] Logs to Logs Aug 5, 2017

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Aug 5, 2017

Contributor

This is ready to be reviewed

Contributor

UziTech commented Aug 5, 2017

This is ready to be reviewed

@50Wliu 50Wliu added needs-review and removed work-in-progress labels Aug 5, 2017

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Aug 9, 2017

Member

I'm starting to suspect this is something with my laptop, but whenever I close the developer tools the Logs dock opens up. Basically the same as atom/github#753.

Member

50Wliu commented Aug 9, 2017

I'm starting to suspect this is something with my laptop, but whenever I close the developer tools the Logs dock opens up. Basically the same as atom/github#753.

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Aug 15, 2017

Contributor

I have quite a few updates to this package (status bar notification count, timeout settings, etc.) that require this pull request to be merged in order to work.

I created a new package with the latest updates since I know it can take a while for a core package to be updated.

notifications-plus

Contributor

UziTech commented Aug 15, 2017

I have quite a few updates to this package (status bar notification count, timeout settings, etc.) that require this pull request to be merged in order to work.

I created a new package with the latest updates since I know it can take a while for a core package to be updated.

notifications-plus

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Aug 16, 2017

Member

@UziTech Ok, gonna try out notifications-plus's 🙈 hard-core mode:

image

Member

simurai commented Aug 16, 2017

@UziTech Ok, gonna try out notifications-plus's 🙈 hard-core mode:

image

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Aug 16, 2017

Contributor

I use it like that with notifications-plus-confetti so I can still see when I get a notification and can click on the count to open the log if I actually want to see what the notification was about. 😃

notifications-plus-confetti

Contributor

UziTech commented Aug 16, 2017

I use it like that with notifications-plus-confetti so I can still see when I get a notification and can click on the count to open the log if I actually want to see what the notification was about. 😃

notifications-plus-confetti

Show outdated Hide outdated lib/main.coffee Outdated
Show outdated Hide outdated lib/main.coffee Outdated
Show outdated Hide outdated lib/notification-element.coffee Outdated
Show outdated Hide outdated lib/notification-element.coffee Outdated
@@ -137,7 +137,7 @@ class NotificationIssue
resolve(@issueBody)
normalizedStackPaths: (stack) =>
stack.replace /(^\W+at )([\w.]{2,} [(])?(.*)(:\d+:\d+[)]?)/gm, (m, p1, p2, p3, p4) => p1 + (p2 or '') +
stack?.replace /(^\W+at )([\w.]{2,} [(])?(.*)(:\d+:\d+[)]?)/gm, (m, p1, p2, p3, p4) => p1 + (p2 or '') +

This comment has been minimized.

@50Wliu

50Wliu Sep 11, 2017

Member

What's the point of this change? I noticed the commit message said fix copying report from string fatal, but I'm not sure when stack can be undefined.

@50Wliu

50Wliu Sep 11, 2017

Member

What's the point of this change? I noticed the commit message said fix copying report from string fatal, but I'm not sure when stack can be undefined.

This comment has been minimized.

@UziTech

UziTech Sep 11, 2017

Contributor

stack is something that is set in the notification options. So it will be undefined if you create a fatal notification programmatically without setting the stack option

e.g.

atom.notifications.addFatalError("fatal"); // no options set ∴ stack is undefined
@UziTech

UziTech Sep 11, 2017

Contributor

stack is something that is set in the notification options. So it will be undefined if you create a fatal notification programmatically without setting the stack option

e.g.

atom.notifications.addFatalError("fatal"); // no options set ∴ stack is undefined
Show outdated Hide outdated lib/notifications-log-item.coffee Outdated
Show outdated Hide outdated lib/notifications-log-item.coffee Outdated
@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 11, 2017

Member

The log doesn't seem to be too happy when in a side dock.
notifications-log-in-side-dock

Member

50Wliu commented Sep 11, 2017

The log doesn't seem to be too happy when in a side dock.
notifications-log-in-side-dock

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Nov 2, 2017

Contributor

Merge conflicts taken care of. 👍 +1 on the clear logs. I could add a command notifications:clear pretty easily if more people are interested.

It would be kind of nice to make atom.notifications.clear() public and have a did-clear-notifications event emitted

Contributor

UziTech commented Nov 2, 2017

Merge conflicts taken care of. 👍 +1 on the clear logs. I could add a command notifications:clear pretty easily if more people are interested.

It would be kind of nice to make atom.notifications.clear() public and have a did-clear-notifications event emitted

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Nov 2, 2017

Contributor

@ungb I added a button to clear the notifications on notifications-plus

Also submitted a pull request to make atom.notifications.clear() public and emit an event atom/atom#16074

Contributor

UziTech commented Nov 2, 2017

@ungb I added a button to clear the notifications on notifications-plus

Also submitted a pull request to make atom.notifications.clear() public and emit an event atom/atom#16074

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Nov 29, 2017

Contributor

@50Wliu is someone reviewing this?

Contributor

UziTech commented Nov 29, 2017

@50Wliu is someone reviewing this?

@leroix leroix self-assigned this Nov 30, 2017

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Nov 30, 2017

Contributor

👋 @UziTech I gotcha fam. Catching up on your changes.

Can you help me understand the problem that this is solving a bit better? What are some situations where not having a notification log becomes particularly painful?

Contributor

leroix commented Nov 30, 2017

👋 @UziTech I gotcha fam. Catching up on your changes.

Can you help me understand the problem that this is solving a bit better? What are some situations where not having a notification log becomes particularly painful?

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Dec 1, 2017

Contributor

@leroix thank you for taking this on.

This pull request helps a few things:

  • Reshow a notification that gets auto-dissmissed before you can finish reading it
  • Click on an auto-dissmissing notification to prevent it from auto-dissmissing
  • Deal with a notification when you have time without keeping it open
Contributor

UziTech commented Dec 1, 2017

@leroix thank you for taking this on.

This pull request helps a few things:

  • Reshow a notification that gets auto-dissmissed before you can finish reading it
  • Click on an auto-dissmissing notification to prevent it from auto-dissmissing
  • Deal with a notification when you have time without keeping it open
@leroix

leroix approved these changes Dec 4, 2017

All of your changes look reasonable to me. I don't see anything major that I would change. Thanks for adding tests, and nice work on the feature! If you're ready, I'll go ahead and merge/publish. 💯

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Dec 4, 2017

Contributor

Ya I have been using it for a while and it has been working great. Let's ship it 👍

Contributor

UziTech commented Dec 4, 2017

Ya I have been using it for a while and it has been working great. Let's ship it 👍

@leroix leroix merged commit 7713fb1 into atom:master Dec 4, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@leroix leroix removed the needs-review label Dec 4, 2017

@@ -79,6 +85,34 @@ Notifications =
@isInitialized = true
togglePanel: ->

This comment has been minimized.

@leroix

leroix Dec 4, 2017

Contributor

@UziTech it looks like this commit reintroduced code that was removed in #177.

...which makes me slightly worried that this may have happened in other places as well.

@leroix

leroix Dec 4, 2017

Contributor

@UziTech it looks like this commit reintroduced code that was removed in #177.

...which makes me slightly worried that this may have happened in other places as well.

This comment has been minimized.

@UziTech

UziTech Dec 4, 2017

Contributor

Ya sorry, that must have been reintroduced on a rebase. I did a quick glance through the code and it looks like that is the only code that was reintroduced.

@UziTech

UziTech Dec 4, 2017

Contributor

Ya sorry, that must have been reintroduced on a rebase. I did a quick glance through the code and it looks like that is the only code that was reintroduced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment