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.

Sign up for GitHub

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

Jump to bottom

Clean up log console api a bit #7379

Merged
merged 42 commits into from Oct 22, 2019
Merged

Clean up log console api a bit #7379

merged 42 commits into from Oct 22, 2019

Conversation

jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Oct 16, 2019 •
edited

References

CC @mbektasbbg

Code changes

This cleans up the api for the log console a bit to conform to conventions we've used elsewhere in the code, such as:

  • passing open options, like is done for creating consoles
  • simplifying the name activeSource to simply source
  • Move max entry limit to the log itself, not just the viewers
  • Fix scrolling to match common log viewer behavior, i.e., when at the bottom, it tails the log, but if you scroll up, it just leaves the scroll position alone.

and many more.

  • Remove (or at least make optional non-default) logging of all notebook output
  • Check sources by explicitly checking for null rather than using truthiness, to guard against an empty source string (we get better typescript checking by explicitly checking against null)

User-facing changes

None

Backwards-incompatible changes

Only in an unreleased package

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jasongrout jasongrout added this to the 2.0 milestone Oct 16, 2019
@jasongrout
Copy link
Contributor Author

@mbektasbbg - if these look good to you, I'll merge and release a 1.2 release candidate.

@jasongrout
Push the max log size down to the model
2da6a4b 
We also threaded the limit through the constructors so that the extension has sole control over the limit.
@jasongrout
Populate console on attachment
2105a18
@jasongrout
Copy link
Contributor Author

@mbektasbbg - I think the functionality is the same now from the user's perspective. Can you review these changes? I'm happy to hop on a call to go over these together if you'd like.

@mbektas
Copy link
Member

mbektas commented Oct 17, 2019

@jasongrout lets have a call to go over the changes as you suggested

mbektas
mbektas approved these changes Oct 17, 2019
View reviewed changes
Copy link
Member

@mbektas mbektas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasongrout these changes look good to me. leaner and cleaner, thanks! I built and tested, haven't seen any issues except new flash color making text not so legible in dark theme.

status-label

@jasongrout
Copy link
Contributor Author

Yeah, I tried to make things consistent with how we are using that info color elsewhere, but it doesn't look so good in a dark theme.

@jasongrout
Simplify the status bar components
6aa7beb
@jasongrout
Change log console highlight to use brand color.
8fd895e 
The info color does not match well with the hardcoded status bar colors, especially in dark mode.
@jasongrout
Copy link
Contributor Author

Thanks @mbektasbbg!

Would you mind looking at the last two commits as well? I simplified the status bar components, and switched the highlight colors to the brand colors.

It seems that it is really hard to highlight something in the status bar because the status bar does not use straight CSS, so you don't get good CSS inheritance.

@jasongrout
Fix scrolling widget initial display to scroll to bottom.
0dd9522
@jasongrout
Fix an initial log console rendering with no source.
f62efaf
@jasongrout
Add todo
0f436d4
@jasongrout
Do more careful handling of null sources.
5c6d18c
@jasongrout
Delete unnecessary todo
d52227d
@jasongrout
Carefully account for message display to fix bugs and remove unnecess…
3fb09a5 
…ary code.

There are some subtle issues fixed here.

1. LogConsolePanel is now more careful and complete in emitting sourceDisplayed signal so that we don’t have to have a message hook anymore.

2. Because we more carefully account for displayed messages, we remove the flashEnabled checks in the status bar updates, which makes the flashing work more correctly. For example, with flashing enabled, now we will see the indicator flash when the console is open but not visible (for example, it may be hidden behind another tab).
@jasongrout
Reorder code to follow JLab guidelines
9bde66c 
* reorder class methods/attributes to public/protected/private
* alphabetically sort imports and some other lists of attributes
@jasongrout
Add todo item about the notebook output logger.
7dbf2cc
@jasongrout
Copy link
Contributor Author

Okay, I think this is good to go now. I left in the notebook output logging so it is easy to test, but we can remove that before merging.

@blink1073 - Would you rather we added a setting somewhere for the notebook to log all output to the log console (default to off)? I think we don't need it for now, but you feel it is really needed, we can add it.

@jasongrout jasongrout mentioned this pull request Oct 21, 2019
Closed
4 tasks
@jasongrout
Copy link
Contributor Author

ping @mbektasbbg for review. My plan is once this is merged, backport it and release another 1.2 alpha, so we can try the ipywidgets changes needed to use it.

@mbektas
Copy link
Member

mbektas commented Oct 21, 2019

@jasongrout I just pulled the latest, rebuilt and tested. Previous issue I reported is gone and I haven't seen any other issues, everything is working great! and with better behavior.

@mbektas
Copy link
Member

mbektas commented Oct 21, 2019 •
edited

I just noticed that log console tab doesn't have an icon anymore. is that intentional or did I miss a build step? var(--jp-icon-output-console) is not set.

@jasongrout
Copy link
Contributor Author

I just noticed that log console tab doesn't have an icon anymore. is that intentional or did I miss a build step

Argh, this probably happened when I moved to the new icon system so we'd have better-themed icons. I'll check into it.

@jasongrout
Copy link
Contributor Author

@telamonian - f34ca0d makes it so that the log console icon shows up in the tab. However, it's always the same color in every theme, which doesn't look very good. Is there a way to get the icon for the tab to change colors in the different themes?

@jasongrout
Copy link
Contributor Author

Let's address the icon color issue in a different PR. This is getting be enough as-is. I'll remove the nb output logging and merge. Thanks @mbektasbbg!

@blink1073 - still would love your thoughts about including the nb output logging.

@jasongrout
Remove notebook output logging.
0d4115d 
Notebook output logging made it easier to test the logging console extension. I think it is too much noise for production, though, unless it is behind a option and turned off by default. See jupyterlab#7386
@jasongrout jasongrout mentioned this pull request Oct 22, 2019
Closed
@jasongrout jasongrout merged commit 2375598 into jupyterlab:master Oct 22, 2019
@jasongrout
Copy link
Contributor Author

@meeseeksdev backport to 1.x

@lumberbot-app
Copy link

lumberbot-app bot commented Oct 22, 2019

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout 1.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 23755988bb1e15ec8473ea6b241735bbd1a9ac9a
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #7379: Clean up log console api a bit'
  1. Push to a named branch :
git push YOURFORK 1.x:auto-backport-of-pr-7379-on-1.x
  1. Create a PR against branch 1.x, I would have named this PR:

"Backport PR #7379 on branch 1.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

jasongrout added a commit to jasongrout/jupyterlab that referenced this pull request Oct 22, 2019
@jasongrout
Backport PR jupyterlab#7379: Clean up log console api a bit
653006d
@jasongrout jasongrout mentioned this pull request Oct 22, 2019
Merged
jasongrout added a commit that referenced this pull request Oct 22, 2019
@jasongrout
Merge pull request #7399 from jasongrout/auto-backport-of-pr-7379-on-1.x
cd55da8 
Backport PR #7379: Clean up log console api a bit
@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Nov 21, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mbektas mbektas mbektas approved these changes

Assignees
No one assigned
Labels
status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jasongrout @mbektas @telamonian @blink1073