Skip to content

/ sqlitebrowser

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

Jump to bottom

Text should be also selectable in "read only" mode #1123

Closed
vlakoff opened this issue Sep 9, 2017 · 18 comments
Closed

Text should be also selectable in "read only" mode #1123

vlakoff opened this issue Sep 9, 2017 · 18 comments
Labels
bug
Milestone
3.11.0 - Where we...

Comments

@vlakoff
Copy link
Contributor

@vlakoff vlakoff commented Sep 9, 2017

I open a database in "read only" mode, then I select a cell. Its content is displayed in the "edit database cell" top right pane. The widget is read only (grayed out), but the text is not selectable, it would be convenient if it were :)
@justinclift
Copy link
Member

@justinclift justinclift commented Sep 9, 2017

Ohhh yeah. That definitely makes sense.

Thanks for reporting it @vlakoff, that should be pretty easy to fix. 😄
@justinclift justinclift added the bug label Sep 9, 2017
@justinclift justinclift added this to the 3.10.1 - Gaaah! milestone Sep 9, 2017
MKleusberg added a commit that referenced this issue Sep 10, 2017
@MKleusberg
Make text selectable in Edit dock even if db is opened as read only
Loading status checks…
fb6ea5a 
Instead of disabling the entire edit dock when the database is opened in
read only mode, only disable all buttons for making changes to the
field. This way the data can still be read using the edit dock.

See issue #1123.
@MKleusberg
Copy link
Member

@MKleusberg MKleusberg commented Sep 10, 2017

This should work in tomorrow's nightly build. Are you ok to download that and test if it's working as expected, @vlakoff? 😄
@vlakoff
Copy link
Contributor Author

@vlakoff vlakoff commented Sep 10, 2017

Sure thing. Apparently it wasn't as trivial to fix as I was hoping.

Will the textarea still be grayed out?
@MKleusberg
Copy link
Member

@MKleusberg MKleusberg commented Sep 10, 2017

No, the text area shouldn't be greyed out anymore in the new version 😄 You won't be able to edit the contents but you should be able to select and copy it.

The change is indeed a bit tricky but not that complicated. If you're interested I can explain a little 😉 The Edit Database Cell dock can be put into three different states: completely disabled, read only mode, and fully enabled. So all I had to do is make sure that it's put into read only mode if the database is opened in read only mode. However, there are tons of different scenarios where we're changing the state of the dialog. It depends on whether a database is opened or not, the active tab, whether you're browsing a table or a view, ... Because of this we not only have to make sure that the dialog is put into read only mode but also that in stays in read only mode. But all that should be done in the next nightly 😄
@vlakoff
Copy link
Contributor Author

@vlakoff vlakoff commented Sep 11, 2017

I have tried it, at first glance it seems to work, I can select text using the mouse.

(fwiw, the pane looks like the "non read-only mode": on first draw the pane has a white background and the text has a grey background, which disappears after I click in the pane)

One caveat though: there is no blinking caret, so notably I can't select text using keyboard Shift + arrows.
MKleusberg added a commit that referenced this issue Sep 18, 2017
@MKleusberg
Make text selectable in Edit dock even if db is opened as read only
Loading status checks…
bbbfb45 
Instead of disabling the entire edit dock when the database is opened in
read only mode, only disable all buttons for making changes to the
field. This way the data can still be read using the edit dock.

See issue #1123.
@justinclift
Copy link
Member

@justinclift justinclift commented Sep 18, 2017

@MKleusberg Should we leave this open and look into the blinking caret thing, or close this?
@MKleusberg
Copy link
Member

@MKleusberg MKleusberg commented Sep 18, 2017

I think we can leave it open and see if we can do something about it. It looks like this is more a Qt issue than a DB4S one but maybe we can work around it somehow. But I haven't tried yet to be honest 😄
@vlakoff
Copy link
Contributor Author

@vlakoff vlakoff commented Sep 19, 2017

Just in case, I asked about it on Stack Overflow.
@justinclift
Copy link
Member

@justinclift justinclift commented Sep 19, 2017

@vlakoff Good thinking. 😄

If you're into IRC, asking on the Qt IRC channel is likely to get helpful info too.
mgrojo added a commit that referenced this issue Dec 5, 2017
@mgrojo
Cell editor fixes and improvements
Loading status checks…
678079a 
Valid JSON is detected as a new first-class data type. See issue #1173.

Text data length is always calculated in characters and not in bytes (UTF-8
gives different results).

Text in read-only mode is also selectable by keyboard and the caret is
visible. See issue #1123
@mgrojo
Copy link
Member

@mgrojo mgrojo commented Dec 5, 2017

@vlakoff I've just applied the answer from Stack Overflow and it works as expected. If you want to take a look at the next nightly build: https://nightlies.sqlitebrowser.org/latest
@justinclift
Copy link
Member

@justinclift justinclift commented Dec 6, 2017

@mgrojo Good stuff. @vlakoff If you have a chance to, would you be ok to test this and confirm if it does/doesn't work well for you now?

@vlakoff As a thought, maybe also let ekhumoro on Stack Overflow know too, as they mention they use DB4S as well and were interested in knowing if it worked. 😄
@vlakoff
Copy link
Contributor Author

@vlakoff vlakoff commented Dec 6, 2017

It seems to work. Just a minor caveat: the caret doesn't blink 😄

Also, as discussed earlier, any chance the textarea background could be made grey again?
@vlakoff
Copy link
Contributor Author

@vlakoff vlakoff commented Dec 9, 2017

I just found this forum thread: http://www.qtcentre.org/threads/39941-readonly-QTextEdit-with-visible-Cursor.

  • post #6: An user uses the same fix as here, and he has a non-blinking caret too.
  • post #7: Another solution is proposed, though it seems a bit heavy...

So, I guess the current solution is the best we can achieve, isn't it?
mgrojo added a commit that referenced this issue Feb 3, 2018
@mgrojo
Set disabled palette for the cell text editor when read-only
Loading status checks…
b8e9637 
In this way the user gets a hint about the text being read-only while still
being able to select text with keyboard and mouse.

See issue #1123
@mgrojo
Copy link
Member

@mgrojo mgrojo commented Feb 3, 2018

@vlakoff I made a change for setting the same palette as the disabled state. It seems to affect only the selection and not the background, but that could be only in my environment. If you'd like to test it, it will be available in the next nightly build.

I don't think that the non-bllinking caret is an issue. That would make me think that I can write, and that wouldn't be true.
@vlakoff
Copy link
Contributor Author

@vlakoff vlakoff commented Feb 4, 2018

I have just tested it, and it seems to work as expected on Windows 7: the whole pane background is grey.

As a reminder, previously I was seeing it as so:

(fwiw, the pane looks like the "non read-only mode": on first draw the pane has a white background and the text has a grey background, which disappears after I click in the pane)
@mgrojo
Copy link
Member

@mgrojo mgrojo commented Feb 5, 2018

Great to know. Then, can we close this issue? Or does anybody still think that the caret should blink?
@vlakoff
Copy link
Contributor Author

@vlakoff vlakoff commented Feb 6, 2018

I think it should be good enough. Text is now selectable, and background is grey, as previously. A handful of code has already been added to achieve this. Going further would be what I call "excess of quality".
@mgrojo
Copy link
Member

@mgrojo mgrojo commented Feb 6, 2018

Ok, @vlakoff Thanks for your feedback.
@mgrojo mgrojo closed this Feb 6, 2018
mgrojo added a commit that referenced this issue Mar 4, 2019
@mgrojo
Removed code for changing the default Qt behaviour for read-only+sele…
e9d8af6 
…ctable

This code was added for #1123 but it isn't working well, some times the
read-only style is used when the cell is writable and vice versa. It has
also problems in the Dark Style mode.

So it is better to remove this code, the read-only characteristic is
hinted by the non-blinking caret. The same should be done for the
QScintilla editors, but it does not have such method.

See issue #1493
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
3.11.0 - Where we take over the world...
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
@justinclift @vlakoff @MKleusberg @mgrojo