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

Clean up log console api a bit #7379

Merged
merged 42 commits into from Oct 22, 2019
Merged

Conversation

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Oct 16, 2019

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

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Oct 16, 2019

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

@jasongrout jasongrout commented Oct 16, 2019

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

@jasongrout jasongrout requested a review from mbektas Oct 16, 2019
jasongrout added 2 commits Oct 17, 2019
We also threaded the limit through the constructors so that the extension has sole control over the limit.
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 17, 2019

@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 mbektas commented Oct 17, 2019

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

Copy link
Member

@mbektas mbektas left a comment

@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

@jasongrout jasongrout commented Oct 17, 2019

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 added 2 commits Oct 18, 2019
The info color does not match well with the hardcoded status bar colors, especially in dark mode.
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 18, 2019

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 added 8 commits Oct 19, 2019
…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).
* reorder class methods/attributes to public/protected/private
* alphabetically sort imports and some other lists of attributes
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 20, 2019

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
4 tasks
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 21, 2019

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 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 mbektas commented Oct 21, 2019

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

@jasongrout jasongrout commented Oct 21, 2019

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

@jasongrout jasongrout commented Oct 22, 2019

@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

@jasongrout jasongrout commented Oct 22, 2019

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.

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 merged commit 2375598 into jupyterlab:master Oct 22, 2019
7 of 9 checks passed
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Oct 22, 2019

@meeseeksdev backport to 1.x

@meeseeksdev
Copy link

@meeseeksdev meeseeksdev 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 issue Oct 22, 2019
jasongrout added a commit that referenced this issue Oct 22, 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.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants