Fixed ‘vs’ command by: #92
Conversation
- Stop using FBInputHandler. This class uses SBInputReader which no longer exist in the lldb version that comes bundled with XCode (that’s why the command was broken), even tho it does exist in the official lldb documentation (http://lldb.llvm.org/python_reference/lldb.SBInputReader-class.html). - Now the script uses the standard stdin library to read from terminal. - Removed the ‘Flickering’ of the selected view, instead we now use mask which is a lot more useful visually.
|
|
||
| viewHelpers.setViewHidden(oldView, False) | ||
|
|
||
| def setCurrentView(self, view): | ||
| def setCurrentView(self, view, oldView): |
There was a problem hiding this comment.
Default oldView to None?
|
@palcalde Thanks! I've left a couple comments. Also, can we make the mask a flag? (You can see examples of how to do flags in other chisel commands.) I'd prefer not to arbitrarily change the command from flickering to masking. Ideally, this could also use a border instead of flicker or mask, but I'm not asking you to go that far 😜 . |
|
@kastiglione thanks for reviewing. I will add a comment about the async mode. The reason is because if you are on lldb console and type vs, the script blocks the thread waiting for user input. That is expected, however, if you type on continue in the debugger XCode enables the simulator again but lldb becomes absolutely blocked. You can't stop the execution or anything like that. The only thing you can do is quit simulator to kill the lldb process. I found that if you run in async mode, lldb runs in a diff thread so it doesn't become blocked when you continue debugger in vs mode. You can stop again and keep using it normally. About the border instead of masking, I will investigate and try to see if I can put it in the open PR. |
To be clear, it's not important to add border support. I'm only concerned about keeping flicker support. I'd rather not remove flicker support, but if we want to augment it with mask support, 👍. For async mode, are there consequences to leaving it in async mode? Or should it be returned to off after |
Removed spaces. Added default None to setCurrentView
|
@kastiglione The only reason I moved flickering to masking is because the new implementation doesn't use SBInputReader, it uses stdin which blocks the thread waiting for user input. SBInputReader was a separated buffer so the previous implementation did a while loop hiding/showing the view every 0.1 sec. Since adding an async implementation with stdin in python is not trivial (it would require a Queue a Thread and a couple of methods) I just thought it wasn't worth it and just went for a simple mask behavior, which is also great to see what view you are on. That is the only reason why I change it :) I added one more commit fixing the issues you mentioned. |
|
|
||
| self.handler = inputHelpers.FBInputHandler(lldb.debugger, self.inputCallback) | ||
| self.handler.start() | ||
| self.setCurrentView(startView, None) |
There was a problem hiding this comment.
Now that oldView defaults to None, the None here can be removed.
|
Thanks for the explanation. Let's do it. A masking I have one more comment, otherwise looks good and thank you so much for the pull request! |
|
Sure, no worries it was fun dealing with lldb and Python a little bit! 👍 |
|
Pull Request updated. |
| print '\nI really have no idea what you meant by \'' + input + '\'... =\\\n' | ||
|
|
||
| viewHelpers.setViewHidden(oldView, False) | ||
| print '\nChisel - VS Mode: I really have no idea what you meant by \'' + input + '\'... =\\\n' |
There was a problem hiding this comment.
Just noticed this. We don't really do this anywhere else with any output. Do you think it's necessary to mention Chisel and VS by name?
|
The only reason why I put that in the message is because is the only command in Chisel (that I know) that stops lldb waiting for user input. I thought it could be useful to remind people they are in vs mode. I can reverse it if you feel more comfortable. Just let me know! |
|
@kastiglione reverted message so is consistent with the rest of commands. |
|
🎆 |
Fixed ‘vs’ command by (Issue #88 (comment)):
longer exist in the lldb version that comes bundled with XCode (that’s
why the command was broken), even tho it does exist in the official
lldb documentation
(http://lldb.llvm.org/python_reference/lldb.SBInputReader-class.html).
mask which is a lot more useful visually.
NOTE: As mentioned this fix doesn't use the class FBInputHandler at all to avoid SBInputReader, but if we have that class back in next versions of XCode we can always revert this fix-commit.