PR: Properly disable editing in Variable Explorer for values in immutable collections (e.g. tuples) #5991
Conversation
|
Looking more into it, this naive approach doesn't account for dates/datetimes and long text strings that are handled separately. Accordingly, I'll amend this commit (ETA later tonight) to deal with those cases too, and do some more hands-on testing. With regard to the latter, I'll see if I can get long text to open to a textbox for easier/more complete viewing, but not actually be editable which can presumably be done through the appropriate methods of the TextEditor object. For dates and times, however, is that currently Spyder does not display datetime objects in a "pretty" format when viewing inline in the variable explorer, only in "edit" mode, so not sure if the best approach is to make it non-editable for now and then in another PR improve the display of dates when not selected (probably not a bad idea anyway at some point), or to enable edit mode as now but just not allow the user to change values, if that is easily implementable. |
|
For long text strings, the fix was quite simple—just pass the readonly argument through to the TextEditor constructor, and bam, read-only text editor to allow full viewing, just not editing. For dates/datetimes, currently I just have them do nothing when edited as I couldn't find an easy way to make the resultant QDate(Time)Edit object, well, uneditable, and aside from looking prettier there appears to be little point. However, I can look into making the static representation of dates and times more pretty/human readable like in the QDate(Time)Editor rather than just the raw format, either in this or a separate PR. |
|
Hello @CAM-Gerlach! Thanks for updating the PR.
Comment last updated on December 26, 2017 at 00:13 Hours UTC |
|
Seeing that the date and datetime editor branches are (now) entirely duplicate code except for one line, I refactored it to unify them. Good idea? Bad idea? I can rollback if you disagree, but I figured I'd put it out there for your feedback. Also, can the pep8speaks warning be ignored, given that I didn't change the format of the comment line and all the others use the same format? |
| if readonly: | ||
| return None | ||
| else: | ||
| if isinstance(value, datetime.datetime): |
There was a problem hiding this comment.
Is datetime.datetime an instance of datetime.date?
There was a problem hiding this comment.
If this is the case, then your refactoring is correct. Else, you have to revert it.
There was a problem hiding this comment.
Yes indeed, and I tested to confirm.
|
Please try to do that in another PR. PRs should stay as simple and focused and possible so we can merge them quickly. |
|
@ccordoba12 Just realized while checking the status of this PR that I must have accidentally edited your comment thinking it was mine, not being used to my new Member powers, x(
Yes indeed; already working on it. Should be simple enough, but well out of scope of this one. [Already done a while ago, PR #6000 ) |
|
I assume I'll want to add tests to check that directly editable values (and perhaps such values inside of sub-objects, lists, etc) really are treated as uneditable when part of an immutable object (e.g. a tuple)? |
|
@CAM-Gerlach, it seems a really hard test to create. I think what you've done so far is really good for now. |
|
Okay. Wouldn't a simple way to unit test my proximate change be to just |
|
Nice! I think it's a good compromise, so please go ahead with it. |
|
Sure, will do... |
|
Is this one ready? |
|
Yup, if you think so. |
There was a problem hiding this comment.
Really nice job with the test you added @CAM-Gerlach! You finally made it work 👍
|
Awesome work @CAM-Gerlach :-) |
|
^_^ Sorry I got a tad frustrated; really, the only tough part where I got stuck was knowing (how) to monkeypatch the relative rather than absolute paths to the modules. After I took a break to get some sleep I figured that had to be it...I just should have taken a step back earlier instead of getting too sucked in, heh. |
Fixes #5953
@ccordoba12 @Prikers
First "real" (code) PR. Before, values in immutable (readonly=True) collections were able to be "edited" in a textbox on doubleclick in a Variable Explorer subwindow, but the changes wouldn't stick (as they shouldn't) once enter/etc was pressed, leading to inconsistency and potential user confusion.
To remedy this, a naive fix as presented here would be to simply check if the readonly property is set when attempting to edit a directly editable value (i.e. not an object that can itself be inspected in a variable explorer subwindow, which might be desired even inside a tuple), and if so, do nothing. Took about 60 seconds to actually write and test the relevant line, and 60 minutes to manually track down where to stick said line...no surprise, lol.
Is this the best approach? Does it break anything else? I ran some simple manual tests with mutable and immutable objects, and viewable objects inside tuples, and everything check out okay, but I'm sure there's edge/corner cases this isn't covering. I rarely do much OO programing, and virtually never anything with a GUI/Qt, so not really sure...
As an aside, I couldn't run the test suite myself since (whether I ran it on the various base branches or on my test branch, directly through
py.test, orruntests.py), the first test would run, a spyder window would open with various actions performed, and then it would stop and get stuck either doing nothing (on, py.test), or on a "Save/Don't Save/Cancel" dialog, and it would either timeout (in the first case), or fail (no matter what I pressed on the dialog, or if I waited several minutes)...not sure what's going on there.Thanks!