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

Fix logpoints bug #7811

Merged
merged 5 commits into from Feb 11, 2019

Conversation

@bomsy
Copy link
Contributor

commented Jan 26, 2019

Issues

  • The log points are shown as conditional breakpoints
  • On editing the log point nothing shows up in the conditional breakpoint panel

logpoints1

  • The context menu for a log point should not show any item relating to conditional breakpoints and vice versa

screenshot 2019-01-25 at 21 51 05

Summary of Changes

  • change log points color to purple
  • cleanup unused menu items
  • fix logValue

Test Plan

Tell us a little a bit about how you tested your patch.

Example test plan:

  • about:config
  • set devtools.debugger.features.log-points to true
  • right click editor gutter menu

Screenshots/Videos (OPTIONAL)

logpoints3

@jasonLaster

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2019

Nice. I like this!

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

Awesome @bomsy ! A few things I noticed:

  1. Create a conditional breakpoint still shows purple:

conditionalcolor

  1. In this PR it would also be sweet if we could fix the context menu for the breakpoints pane; we still show "Add condition":

conditioncontext

It seems like it fits in with your other context menu fix.

  1. Test failure looks legit, I think because we removed the context menu item.
@bomsy

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

Thanks for reviewing @darkwing. Will update asap.

@bomsy bomsy force-pushed the bomsy:fix-logpoints-bug branch 3 times, most recently from c855e28 to 131b105 Feb 6, 2019

@jasonLaster jasonLaster force-pushed the bomsy:fix-logpoints-bug branch 3 times, most recently from 5a3c916 to 80a5852 Feb 10, 2019

@bomsy bomsy requested a review from flodolo as a code owner Feb 10, 2019

@jasonLaster jasonLaster force-pushed the bomsy:fix-logpoints-bug branch from 80a5852 to 5d3237c Feb 10, 2019

@jasonLaster

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2019

Okay, this ux is definitely too vague... I took a stab at handling a couple of cases:

  1. context menus (gutter, column, panel)
  2. marker colors
  3. panel bp text

The heuristics I used were:

  1. treat logs and conditions as independent (can be added / removed / edited)
  2. give precedence to logs (highlight purple, show bp text)

Adding a new bp

screen shot 2019-02-10 at 9 35 44 am

with a condition

screen shot 2019-02-10 at 9 36 20 am

with a log

screen shot 2019-02-10 at 9 37 47 am

with both set

screen shot 2019-02-10 at 9 38 02 am

@@ -534,6 +534,13 @@ editor.addLogPoint.accesskey=l
# LOCALIZATION NOTE (editor.editLogPoint): Editor gutter context menu item
# for editing a log point already set on a line.
editor.editLogPoint=Edit log
editor.editLogPoint.accesskey=e

This comment has been minimized.

Copy link
@flodolo

flodolo Feb 10, 2019

E (better to respect the case)


# LOCALIZATION NOTE (editor.removeLogPoint): Context menu item for removing
# a log point on a line.
editor.removeLogPoint=Remove log

This comment has been minimized.

Copy link
@flodolo

flodolo Feb 10, 2019

nit: can we call this editor.removeLogPoint.label?

# LOCALIZATION NOTE (editor.removeLogPoint): Context menu item for removing
# a log point on a line.
editor.removeLogPoint=Remove log
editor.removeLogPoint.accesskey=p

This comment has been minimized.

Copy link
@flodolo

flodolo Feb 10, 2019

Why the external accesskey (not available in the label)?

This comment has been minimized.

Copy link
@jasonLaster

jasonLaster Feb 10, 2019

Contributor

i don't follow?

This comment has been minimized.

Copy link
@flodolo

flodolo Feb 11, 2019

Label "Remove log", there's no "p", which will result in "Remove log (p)" (with p underscored) on Windows and Linux.

@jasonLaster jasonLaster force-pushed the bomsy:fix-logpoints-bug branch 3 times, most recently from 798fae9 to 341aa7f Feb 10, 2019

@jasonLaster jasonLaster force-pushed the bomsy:fix-logpoints-bug branch from 341aa7f to 9710ad7 Feb 11, 2019

@jasonLaster jasonLaster merged commit dc1317e into firefox-devtools:master Feb 11, 2019

0 of 2 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.