Generate "All magics..." menu live #956

Merged
merged 5 commits into from Nov 24, 2011

Conversation

Projects
None yet
2 participants
@Carreau
Member

Carreau commented Oct 31, 2011

Hi everyone,

I'm trying to catch up with the merging of the qtconsole. For now constructing the "all magic menu" by querying the kernel about get_ipython().lsmagic() through user_expressions. I thought it might be a good idea.

It does work as long as the action is not automatically done at startup, otherwise there is no prompt.
( try adding :self.window.pop.trigger() just before self.app._exec() in qtconsole.py )
You can trigger the action by hand through the Magics > Populate All Magic Menu Menu, which create a second All Magics Menu.

I don't know why, it's not working at startup, and i'm not sure adding a timer to execute this action later is a good idea.
I was also thinking of updating at each new prompt, or every time the user click on Magic Menu.

If can get a little help on this ...

Otherwise, still not 100% back, still digging in my backups to recover some data, but will do my best.

Matthias

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Nov 4, 2011

Member

Hi,

Found what I was looking for. I've boud the action to update the menu to a first_response signal.
The menu is now populated with all the magics from %lsmagic.

I've also append '?' to the end of some magic which failed when invoked alone.

Member

Carreau commented Nov 4, 2011

Hi,

Found what I was looking for. I've boud the action to update the menu to a first_response signal.
The menu is now populated with all the magics from %lsmagic.

I've also append '?' to the end of some magic which failed when invoked alone.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Nov 10, 2011

Member

rebased on master, and forced push..

Member

Carreau commented Nov 10, 2011

rebased on master, and forced push..

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Nov 11, 2011

Member

Glad to have you back @Carreau. I'll try to review this over the weekend, if nobody beats me to it.

Member

fperez commented Nov 11, 2011

Glad to have you back @Carreau. I'll try to review this over the weekend, if nobody beats me to it.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Nov 11, 2011

Member

I found a bit of time to do it now :)

I like this and I think it's useful, as it will encourage users to browse around and try some magics they might not know about. It also seems to work perfectly on my system. I'll add some inline comments that are fairly minor code tidying issues and should be pretty easy to address.

Member

fperez commented Nov 11, 2011

I found a bit of time to do it now :)

I like this and I think it's useful, as it will encourage users to browse around and try some magics they might not know about. It also seems to work perfectly on my system. I'll add some inline comments that are fairly minor code tidying issues and should be pretty easy to address.

@fperez

View changes

IPython/frontend/qt/console/frontend_widget.py
@@ -137,6 +138,7 @@ class FrontendWidget(HistoryConsoleWidget, BaseFrontendMixin):
self._input_splitter = self._input_splitter_class(input_mode='cell')
self._kernel_manager = None
self._request_info = {}
+ self._callback_dict=dict([])

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

Create the emtpy dict with {} like the line above, for consistency.

@fperez

fperez Nov 11, 2011

Member

Create the emtpy dict with {} like the line above, for consistency.

@fperez

View changes

IPython/frontend/qt/console/frontend_widget.py
@@ -311,6 +313,42 @@ class FrontendWidget(HistoryConsoleWidget, BaseFrontendMixin):
cursor.movePosition(QtGui.QTextCursor.Left, n=len(text))
self._complete_with_items(cursor, rep['content']['matches'])
+ def _silent_exec_callback(self,expr,callback):

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

pep-8: use a space after each comma

@fperez

fperez Nov 11, 2011

Member

pep-8: use a space after each comma

@fperez

View changes

IPython/frontend/qt/console/frontend_widget.py
@@ -311,6 +313,42 @@ class FrontendWidget(HistoryConsoleWidget, BaseFrontendMixin):
cursor.movePosition(QtGui.QTextCursor.Left, n=len(text))
self._complete_with_items(cursor, rep['content']['matches'])
+ def _silent_exec_callback(self,expr,callback):
+ """ silently execute a function in the kernel and send the reply to the callback

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

Docstring formatting: Capitalize properly all sentences, no extra spaces at the beginning of each sentence, and use the formatting guidelines for docstring parameters. We follow the numpy docstring standard, as documented in our developer guide.

Once ytou get used to following proper formatting conventions, they become second nature. And since we follow the same standard as numpy, scipy and many other projects in the 'scipy ecosystem', it will be a good habit to pick up.

@fperez

fperez Nov 11, 2011

Member

Docstring formatting: Capitalize properly all sentences, no extra spaces at the beginning of each sentence, and use the formatting guidelines for docstring parameters. We follow the numpy docstring standard, as documented in our developer guide.

Once ytou get used to following proper formatting conventions, they become second nature. And since we follow the same standard as numpy, scipy and many other projects in the 'scipy ecosystem', it will be a good habit to pick up.

+ # generate uuid, which would be used as a indication of wether or not
+ # the unique request originate from here (can use msg id ?)
+ local_uuid = str(uuid.uuid1())
+ msg_id = self.kernel_manager.shell_channel.execute('',

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

No need to break up this call into so many extra lines. You should keep individual lines to less than 80 characters, but otherwise you can finish up the dict and closing parens all in one line, for example:

msg_id = self.kernel_manager.shell_channel.execute('', 
            silent=True, user_expressions={ local_uuid:expr } )
@fperez

fperez Nov 11, 2011

Member

No need to break up this call into so many extra lines. You should keep individual lines to less than 80 characters, but otherwise you can finish up the dict and closing parens all in one line, for example:

msg_id = self.kernel_manager.shell_channel.execute('', 
            silent=True, user_expressions={ local_uuid:expr } )
@fperez

View changes

IPython/frontend/qt/console/frontend_widget.py
+ user_expressions={ local_uuid:expr,
+ }
+ )
+ self._callback_dict[local_uuid]=callback

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

pep-8: spaces around = sign missing.

@fperez

fperez Nov 11, 2011

Member

pep-8: spaces around = sign missing.

@fperez

View changes

IPython/frontend/qt/console/frontend_widget.py
+ self._callback_dict[local_uuid]=callback
+ self._request_info['execute'] = self._ExecutionRequest(msg_id, 'silent_exec_callback')
+
+ def _handle_exec_callback(self,msg):

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

pep8: space after comma

@fperez

fperez Nov 11, 2011

Member

pep8: space after comma

@fperez

View changes

IPython/frontend/qt/console/frontend_widget.py
+ self._request_info['execute'] = self._ExecutionRequest(msg_id, 'silent_exec_callback')
+
+ def _handle_exec_callback(self,msg):
+ """ Called when _silent_exec_callback message comme back from the kernel.

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

Docstring should follow standard, see above.

@fperez

fperez Nov 11, 2011

Member

Docstring should follow standard, see above.

@fperez

View changes

IPython/frontend/qt/console/frontend_widget.py
+ Strip the message comming back from the kernel and send it to the
+ corresponding callback function.
+ """
+ cnt=msg['content']

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

spaces around = signs here and elsewhere, won't repeat it in every line but check the whole diff and fix everywhere (though read pep8, they are not to be added in keyword args).

@fperez

fperez Nov 11, 2011

Member

spaces around = signs here and elsewhere, won't repeat it in every line but check the whole diff and fix everywhere (though read pep8, they are not to be added in keyword args).

@fperez

View changes

IPython/frontend/qt/console/frontend_widget.py
+ """
+ cnt=msg['content']
+ ue=cnt['user_expressions']
+ for i in ue.keys():

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

A dict can be iterated without calling .keys(), which is both clearer and faster, as it prevents the creation of an unnecessary list. Simply use for i in ue:.

Argh, never mind. I hadn't noticed you were modifying the dict, in that case you do need to create the static keys list. Ignore the above.

@fperez

fperez Nov 11, 2011

Member

A dict can be iterated without calling .keys(), which is both clearer and faster, as it prevents the creation of an unnecessary list. Simply use for i in ue:.

Argh, never mind. I hadn't noticed you were modifying the dict, in that case you do need to create the static keys list. Ignore the above.

@fperez

View changes

IPython/frontend/qt/console/frontend_widget.py
+ for i in ue.keys():
+ if i in self._callback_dict.keys():
+ f= self._callback_dict[i]
+ f(ue[i])

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

No need to create a separate f variable here just to call it right away, simply write self._callback_dict[i](ue[i])

@fperez

fperez Nov 11, 2011

Member

No need to create a separate f variable here just to call it right away, simply write self._callback_dict[i](ue[i])

@fperez

View changes

IPython/frontend/qt/console/frontend_widget.py
+ cnt=msg['content']
+ ue=cnt['user_expressions']
+ for i in ue.keys():
+ if i in self._callback_dict.keys():

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

Never test dict membership via the keys() list, which is an O(n) operation, use instead if i in self._callback_dict. It's shorter to type, clearer and O(1), so potentially much much faster.

@fperez

fperez Nov 11, 2011

Member

Never test dict membership via the keys() list, which is an O(n) operation, use instead if i in self._callback_dict. It's shorter to type, clearer and O(1), so potentially much much faster.

@fperez

View changes

IPython/frontend/qt/console/mainwindow.py
@@ -544,10 +544,44 @@ class MainWindow(QtGui.QMainWindow):
self.kernel_menu.addSeparator()
+ def update_all_magic_menu(self):
+ # first define a callback which will get the list of all magic and put it in the menu.

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

Docstring for method missing.

@fperez

fperez Nov 11, 2011

Member

Docstring for method missing.

@fperez

View changes

IPython/frontend/qt/console/mainwindow.py
+ alm_magic_menu = self.all_magic_menu
+ alm_magic_menu.clear()
+ def make_dynamic_magic(i):
+ def inner_dynamic_magic():

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

Ouch, you have a 4-level deep nested local function definition! That kind of code is very hard to read and analyze, because of the amount of nested scopes one must track. Can you try to simplify this a little bit to use a flatter code, hopefully with zero or at most one locally defined inner function?

@fperez

fperez Nov 11, 2011

Member

Ouch, you have a 4-level deep nested local function definition! That kind of code is very hard to read and analyze, because of the amount of nested scopes one must track. Can you try to simplify this a little bit to use a flatter code, hopefully with zero or at most one locally defined inner function?

@fperez

View changes

IPython/frontend/qt/console/mainwindow.py
+
+ # list of protected magic that don't like to be called without argument
+ # append '?' to the end to print the docstring when called from the menu
+ protected_magic= ["more","less","load_ext","pycat","loadpy","save"]

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

If you are going to be testing for membership in this set, you should make it a set() instead of a list with

protected_magic= set(["more","less","load_ext","pycat","loadpy","save"])

which will make the membership checks much faster.

@fperez

fperez Nov 11, 2011

Member

If you are going to be testing for membership in this set, you should make it a set() instead of a list with

protected_magic= set(["more","less","load_ext","pycat","loadpy","save"])

which will make the membership checks much faster.

+ # this action should not appear as it will be cleard when menu
+ # will be updated at first kernel response.
+ self.pop = QtGui.QAction("&Update All Magic Menu ",
+ self,

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

this can fit into the line above, no need to break it up into so many.

@fperez

fperez Nov 11, 2011

Member

this can fit into the line above, no need to break it up into so many.

@fperez

View changes

IPython/frontend/qt/console/qtconsoleapp.py
@@ -451,6 +451,10 @@ class IPythonQtConsoleApp(BaseIPythonApplication):
self.window.log = self.log
self.window.add_tab_with_frontend(self.widget)
self.window.init_menu_bar()
+
+ #we need to populate the 'Magic Menu' once the kernel has answer at least once

This comment has been minimized.

@fperez

fperez Nov 11, 2011

Member

pep8: Leave a space after the # comment marker.

@fperez

fperez Nov 11, 2011

Member

pep8: Leave a space after the # comment marker.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Nov 11, 2011

Member

OK, I've left a bunch of feedback. Most of it is easy to address and mostly code formatting/documentation issues that you'll get used to quickly. There's only one issue that probably will require some code refactoring.

But overall it's a nice improvement, thanks again!

Member

fperez commented Nov 11, 2011

OK, I've left a bunch of feedback. Most of it is easy to address and mostly code formatting/documentation issues that you'll get used to quickly. There's only one issue that probably will require some code refactoring.

But overall it's a nice improvement, thanks again!

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Nov 11, 2011

Member

@fperez,
Just pushed a squashed commit of all the suggestion you made, I was a little lazy about code formating.
If you are ok with that I can push a squashed commit of everything rebased on the last master.

Member

Carreau commented Nov 11, 2011

@fperez,
Just pushed a squashed commit of all the suggestion you made, I was a little lazy about code formating.
If you are ok with that I can push a squashed commit of everything rebased on the last master.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Nov 11, 2011

Member

Sure, go ahead and force push a cleaned up commit if you prefer. I need to tune out of ipython for the day, I have some local work I must tend to now. But I'll get back to this as soon as I can, probably tomorrow.

Member

fperez commented Nov 11, 2011

Sure, go ahead and force push a cleaned up commit if you prefer. I need to tune out of ipython for the day, I have some local work I must tend to now. But I'll get back to this as soon as I can, probably tomorrow.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Nov 12, 2011

Member

Squashed, rebased on master, and forced pushed ...

Member

Carreau commented Nov 12, 2011

Squashed, rebased on master, and forced pushed ...

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Nov 16, 2011

Member

Rebase on master agin to stay up to date....

Member

Carreau commented Nov 16, 2011

Rebase on master agin to stay up to date....

@fperez

View changes

IPython/frontend/qt/console/mainwindow.py
+ # append '?' to the end to print the docstring when called from the menu
+ protected_magic = set(["more","less","load_ext","pycat","loadpy","save"])
+
+ for magic in eval(listofmagic):

This comment has been minimized.

@fperez

fperez Nov 19, 2011

Member

Why are you using eval here? Eval is a fairly dangerous construct that should only be necessary in very specialized places, I don't see why it's needed here...

@fperez

fperez Nov 19, 2011

Member

Why are you using eval here? Eval is a fairly dangerous construct that should only be necessary in very specialized places, I don't see why it's needed here...

This comment has been minimized.

@Carreau

Carreau Nov 19, 2011

Member

you'r right, it can be done by regExp ... that was because the return value by the kernel was the repr() of the list, so I thought to use eval without trying harder

@Carreau

Carreau Nov 19, 2011

Member

you'r right, it can be done by regExp ... that was because the return value by the kernel was the repr() of the list, so I thought to use eval without trying harder

@fperez

View changes

IPython/frontend/qt/console/mainwindow.py
+
+ def update_all_magic_menu(self):
+ # first define a callback which will get the list of all magic and put it in the menu.
+ self.active_frontend._silent_exec_callback('get_ipython().lsmagic()',self.populate_all_magic_menu)

This comment has been minimized.

@fperez

fperez Nov 19, 2011

Member

pep8, space after comma

@fperez

fperez Nov 19, 2011

Member

pep8, space after comma

This comment has been minimized.

@Carreau

Carreau Nov 19, 2011

Member

missed that one...

@Carreau

Carreau Nov 19, 2011

Member

missed that one...

@fperez

View changes

IPython/frontend/qt/console/frontend_widget.py
+ def _silent_exec_callback(self, expr, callback):
+ """Silently execute `expr` in the kernel and call `callback` with reply
+
+ `expr` : valid string to be executed by the kernel.

This comment has been minimized.

@fperez

fperez Nov 19, 2011

Member

Thanks for updating the docstrings, but please make them conforming the numpy standard, these do not. Our development docs have an explanation. While this one in particular is a private method so it can be kept shorter, if you're going to document the arguments (which is great!), then it should be done in compliance with the rest of the codebase.

In short, they need to read

One line description.

more text
if necessary

Parameters
----------
  foo : str
    a string that is ...

  bar : int
    blah blah...

Returns
-------
x : blah...

Just have a look at the link above; once you get used to it they'll become second nature.

@fperez

fperez Nov 19, 2011

Member

Thanks for updating the docstrings, but please make them conforming the numpy standard, these do not. Our development docs have an explanation. While this one in particular is a private method so it can be kept shorter, if you're going to document the arguments (which is great!), then it should be done in compliance with the rest of the codebase.

In short, they need to read

One line description.

more text
if necessary

Parameters
----------
  foo : str
    a string that is ...

  bar : int
    blah blah...

Returns
-------
x : blah...

Just have a look at the link above; once you get used to it they'll become second nature.

@fperez

View changes

IPython/frontend/qt/console/frontend_widget.py
+
+ cnt = msg['content']
+ ue = cnt['user_expressions']
+ for i in ue.keys():

This comment has been minimized.

@fperez

fperez Nov 19, 2011

Member

You can just iterate on ue itself: for i in ue. The dict that's getting modified is _callback_dict, so no need to build an explicit keys list here. You only need the explicit keys list if you want to simultaneously iterate and modify in-place the same dict.

@fperez

fperez Nov 19, 2011

Member

You can just iterate on ue itself: for i in ue. The dict that's getting modified is _callback_dict, so no need to build an explicit keys list here. You only need the explicit keys list if you want to simultaneously iterate and modify in-place the same dict.

@fperez

View changes

IPython/frontend/qt/console/frontend_widget.py
+ for i in ue.keys():
+ if i in self._callback_dict:
+ self._callback_dict[i](ue[i])
+ self._callback_dict.pop(i)

This comment has been minimized.

@fperez

fperez Nov 19, 2011

Member

pop returns the popped value, so these two lines should be replaced with just

self._callback_dict.pop(i)(ue[i])
@fperez

fperez Nov 19, 2011

Member

pop returns the popped value, so these two lines should be replaced with just

self._callback_dict.pop(i)(ue[i])
+ return inner_dynamic_magic
+
+ def populate_all_magic_menu(self, listofmagic=None):
+ """Clean "All Magics..." menu and repopulate it with `listofmagic`

This comment has been minimized.

@fperez

fperez Nov 19, 2011

Member

Same comments as above on docstring formatting.

@fperez

fperez Nov 19, 2011

Member

Same comments as above on docstring formatting.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Nov 19, 2011

Member

Thanks, I think we're getting close! Just a few more fixes and we should be good to merge.

Member

fperez commented Nov 19, 2011

Thanks, I think we're getting close! Just a few more fixes and we should be good to merge.

Carreau added some commits Oct 30, 2011

Generate "All magics..." menu live
	this use 'user_expression' request to ask for the value of
	'get_ipython().lsmagic()' and populate the menu with the result.

	As is, the action need to be triggerd by hand, once the console
	have appear. Otherwith there is no prompt

	The logic can certainly also be shortend by using msg.id
	instead of uuid

	We should also keep a list of magic that shouldn't be executed without
	argument (eg: %gui) and don't put them in the menu. or appending '?' at the
	end
triger menu update when kernel started
	bind the update action to shell_channel's first_response event.
	add a list of magic that don't like beeing called alone ( like
	%more , %pycat , ...Etc) and append '?' in the menu to the end
	to show docsctring when clicked
	This list should be completed in :
		IPython/frontend/qt/console/mainwindow.py
fix docstrig, replace eval by regExp
	replace the `eval` to transform the `repr()` of the magic list send by the kernel
	by a regular expression, change the docstring to be numpy complient (I hope so)
@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Nov 24, 2011

Member

Sooooory ... i fixed everything 2 or 3 days ago and forgot to push...
I rebased and foced push...

Member

Carreau commented Nov 24, 2011

Sooooory ... i fixed everything 2 or 3 days ago and forgot to push...
I rebased and foced push...

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Nov 24, 2011

Member

Great, very nice! Thanks for the excellent contribution, merging now. Everything looks good here.

Member

fperez commented Nov 24, 2011

Great, very nice! Thanks for the excellent contribution, merging now. Everything looks good here.

fperez added a commit that referenced this pull request Nov 24, 2011

Merge pull request #956 from Carreau/all-magic-menu-live
Generate "All magics..." menu in the Qt console dynamically, so it correctly shows available magics instead of an internally hardcoded list.

@fperez fperez merged commit 8fa6544 into ipython:master Nov 24, 2011

fperez added a commit that referenced this pull request Nov 28, 2011

Temporary fix to work around #1057.
Basically it reverts the effect of #956 and goes back to a static list
for the 'all magics' menu.  I tried to mark very clearly the new code
so we can disable it once a proper fix for #1057 is committed, but we
can't have a broken qt console in master.

Carreau added a commit to Carreau/ipython that referenced this pull request Nov 29, 2011

Revert "Temporary fix to work around #1057."
    This reverts commit 65546bf, done to
    temporaly fixed a race condition introduced by #956, next commits should
    fixe this race condition

Carreau added a commit to Carreau/ipython that referenced this pull request Nov 29, 2011

qtconsole: fix race-cond, handle multiple exec
	fix race condition in qtconsole due to a race condition beween the population
	of the magic menu and the first prompt request. Resolved by upgrading the
	logic of the console to handle several executions request in parallel.

	Some namedTuple might still carry redundent information but the fix is the
	first priority

	fixes #1057 , introduced by #956

fperez added a commit that referenced this pull request Nov 30, 2011

Merge pull request #1065 from Carreau/qtconsole-racecondition
Fix race condition in qtconsole between the population of the magic menu and the first prompt request. 

Resolved by upgrading the logic of the console to handle several executions request in parallel.

Closes #1057, introduced by #956.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #956 from Carreau/all-magic-menu-live
Generate "All magics..." menu in the Qt console dynamically, so it correctly shows available magics instead of an internally hardcoded list.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Temporary fix to work around #1057.
Basically it reverts the effect of #956 and goes back to a static list
for the 'all magics' menu.  I tried to mark very clearly the new code
so we can disable it once a proper fix for #1057 is committed, but we
can't have a broken qt console in master.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Revert "Temporary fix to work around #1057."
    This reverts commit 65546bf, done to
    temporaly fixed a race condition introduced by #956, next commits should
    fixe this race condition

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

qtconsole: fix race-cond, handle multiple exec
	fix race condition in qtconsole due to a race condition beween the population
	of the magic menu and the first prompt request. Resolved by upgrading the
	logic of the console to handle several executions request in parallel.

	Some namedTuple might still carry redundent information but the fix is the
	first priority

	fixes #1057 , introduced by #956

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #1065 from Carreau/qtconsole-racecondition
Fix race condition in qtconsole between the population of the magic menu and the first prompt request. 

Resolved by upgrading the logic of the console to handle several executions request in parallel.

Closes #1057, introduced by #956.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment