PR: Add an option to set cursor blinking speed in miliseconds #4758

jnsebgosselin · 2017-07-18T02:31:28Z

goanpeca · 2017-07-18T02:59:01Z

@jnsebgosselin thanks for the contribution, you need to align the two spinboxes (the one from custom margins from panes) Look at the code that does this for the fonts on upper part of your screenshot

goanpeca · 2017-07-18T02:59:24Z

@jnsebgosselin also you should make the PR against 3.x I think @ccordoba12 ?

Note: I had to connect box toggled signal to both spinbox and slabel setEnabled slot since custom merged spinbox layout becomes irrelevant as soon as the spinbox and slabel are added individually in the layout (for alignment purpose).

jnsebgosselin · 2017-07-18T03:44:20Z

Preview with aligned custom margins and caret blinking spinboxes and slabels:

goanpeca · 2017-07-18T03:45:19Z

Looks good, except for the branch @ccordoba12 ?

ccordoba12 · 2017-07-18T03:47:07Z

@jnsebgosselin, thanks a lot for your contributions! However, we need to ask you to move all your branches from the 3.1.x branch (which is not going to receive more updates) to the 3.x branch (which is our maintenance branch right now).

Sorry for not being clear enough since your first contribution, but you can find the instructions to do that here

https://github.com/spyder-ide/spyder/blob/master/CONTRIBUTING.md#spyder-branches

Note: I had to connect box toggled signal to both spinbox and slabel setEnabled slot since custom merged spinbox layout becomes irrelevant as soon as the spinbox and slabel are added individually in the layout (for alignment purpose).

…t_blinking_option # Conflicts: # spyder/config/main.py # spyder/plugins/configdialog.py

jnsebgosselin · 2017-07-18T05:15:38Z

@ccordoba12 kk good. I finally managed to rebase this PR. I hope it is not too messy. I can create a new clean PR if required.

Thanks for the link, I'll read it carefully and I will try to improve my setup.

ccordoba12 · 2017-07-30T15:54:15Z

spyder/app/mainwindow.py

@@ -2539,6 +2539,11 @@ def apply_settings(self):

        self.apply_panes_settings()
        self.apply_statusbar_settings()
+
+        if CONF.get('main', 'use_custom_caret_blinking') is True:


There's no need for is True at the end of this line, so please remove it.

ccordoba12 · 2017-07-30T15:56:07Z

spyder/config/main.py

@@ -93,6 +93,8 @@
              'cpu_usage/timeout': 2000,
              'use_custom_margin': True,
              'custom_margin': 0,
+              'use_custom_caret_blinking': False,
+              'custom_caret_blinking': 1000,


Instead of using caret as part of these options, please use cursor instead, i.e. use_custom_cursor_blinking and custom_cursor_blinking.

ccordoba12 · 2017-07-30T15:57:55Z

Changing this option requires a Spyder restart because it's applied at the window level.

@rlaverde, please instruct @jnsebgosselin on how to do that.

rlaverde · 2017-07-31T14:23:50Z

@jnsebgosselin Thanks for you contribution, in order to make this a restarting option, you need to add restart support to create_spinbox() and create_checkbox, and in def load_from_conf() add the labels to the restart_options array. And use it when calling the creating of the checkbox and he spinbox

You could take a look at this commit 8fafe45

QApplication cursorFlashTime is cached on startup so that the default value of the OS can be used if the user unchecked `Cursor blinking` option in preferences. This is needed since QApplication.cursorFlashTime() returns the value of the current QApplication instance, not the OS default value.

jnsebgosselin · 2017-07-31T21:01:43Z

@rlaverde @ccordoba12 Thank you. But are you sure this needs a restart? Seems to me like this can be applied at run-time. At least, it is on Windows.

That being said, something was not working as expected with the code. When the option to use a custom cursor blinking time was unchecked, after a custom value had been set, the value was not reset to the OS default. QApplication.cursorFlashTime() actually returns the value for the current application instance, not the default value set for the OS. So basically, nothing was being changed.

I haven't found an easy way to get the OS default value for the cursor flash time. This seems to be very platform dependent. Unless you guys know of a better way, maybe we could cache the value of QApplication.cursorFlashTime() at startup?This way, no restart would be needed for this setting, which would be better I think. I've made the changes to the PR so you can look it up already.

goanpeca · 2017-07-31T21:09:50Z

spyder/config/main.py

@@ -93,6 +93,8 @@
              'cpu_usage/timeout': 2000,
              'use_custom_margin': True,
              'custom_margin': 0,
+              'use_custom_cursor_blinking': False,
+              'custom_cursor_blinking': 1000,


But this value should be used then 'custom_cursor_blinking': CURSORBLINK_OSDEFAULT, no?

You are right. This makes sense to set the default value here as the default value for the OS. I will change it.

@goanpeca in the header of this file (main.py), it is written:

Note: Leave this file free of Qt related imports [...]

So I don't know how I can link CURSORBLINK_OSDEFAULT, whose value must be obtained with QApplication... Do you have any idea? Maybe another solution could be to set the default value when creating the spinbox within the configdialog.py module instead of here? Something like:

cursor_spin = self.create_spinbox( "", _("ms"), 'custom_cursor_blinking', default = QApplication.cursorFlashTime(), min_ = 0, max_ = 5000)

@jnsebgosselin, your solution is the only one that occurs to me too.

Since we can't adjust this value in our config system, please remove it from here. It'll be written to spyder.ini when the spinbox is created.

goanpeca · 2017-08-01T14:14:32Z

@ccordoba12 ?

ccordoba12 · 2017-08-01T16:52:17Z

spyder/plugins/configdialog.py

@@ -887,7 +889,8 @@ def setup_page(self):
        cursor_box = newcb(_("Cursor blinking:"),
                           'use_custom_cursor_blinking')
        cursor_spin = self.create_spinbox("", _("ms"), 'custom_cursor_blinking',
-                                          0, 0, 5000)
+                                          default = QApplication.cursorFlashTime(),
+                                          min_ = 0, max_ = 5000, step = 10)


Please remove spaces around =. They are not needed for keyword args.

ccordoba12 · 2017-08-01T16:52:37Z

spyder/plugins/configdialog.py

@@ -328,6 +329,7 @@ def load_from_conf(self):
            if lineedit.restart_required:
                self.restart_options[option] = lineedit.label_text
        for spinbox, (option, default) in list(self.spinboxes.items()):
+            print(option, default, self.get_option(option, default))


Please remove this print here.

goanpeca · 2017-08-01T16:57:41Z

spyder/config/main.py

@@ -94,7 +94,6 @@
              'use_custom_margin': True,
              'custom_margin': 0,
              'use_custom_cursor_blinking': False,
-              'custom_cursor_blinking': 1000,


@ccordoba12 and @jnsebgosselin I think we still need to have the option here, the only difference is that it would be None by default, and when building the spinbox, we would check if it is None and update with the default by QApplication, or just use the stored value.

If I do that, then delete my .init files and restart Spyder, I get the following traceback:

Traceback (most recent call last): File "C:\Users\jsgosselin\spyder\spyder\app\mainwindow.py", line 2585, in edit_preferences widget.initialize() File "C:\Users\jsgosselin\spyder\spyder\plugins\configdialog.py", line 76, in initialize self.load_from_conf() File "C:\Users\jsgosselin\spyder\spyder\plugins\configdialog.py", line 332, in load_from_conf spinbox.setValue(self.get_option(option, default)) TypeError: setValue(self, int): argument 1 has unexpected type 'NoneType'

@goanpeca, there is no need to do that because values are placed here only if we plan to change/update them in the future.

But this value is OS dependent, so we can't put it here.

andfoy · 2017-08-25T22:35:23Z

@ccordoba12 This is working as expected, both on Windows and Linux, without any Spyder reboot required

ccordoba12 · 2017-08-26T17:29:49Z

Thanks a lot for testing @andfoy! Merging then ;-)

Fixes #4277

Added an option to set caret cursor blinking speed in ms

5e29849

goanpeca self-requested a review July 18, 2017 02:58

jnsebgosselin added 4 commits July 18, 2017 00:42

Added an option to set caret cursor blinking speed in ms

1e86648

Removed line added by mistake when resolving conflicts

4d61e09

Merge remote-tracking branch 'origin/caret_blinking_option' into care…

d88b57a

…t_blinking_option # Conflicts: # spyder/config/main.py # spyder/plugins/configdialog.py

jnsebgosselin changed the base branch from 3.1.x to 3.x July 18, 2017 05:05

ccordoba12 reviewed Jul 30, 2017

View reviewed changes

ccordoba12 added this to the v3.2.2 milestone Jul 30, 2017

jnsebgosselin added 3 commits July 31, 2017 12:16

Replaced all "caret" words by "cursor"

46f5a70

removed not needed is True

0a8b97a

goanpeca changed the title ~~Add an option to set caret cursor blinking speed in ms~~ Add an option to set cursor cursor blinking speed in ms Jul 31, 2017

goanpeca changed the title ~~Add an option to set cursor cursor blinking speed in ms~~ Add an option to set cursor blinking speed in ms Jul 31, 2017

goanpeca reviewed Jul 31, 2017

View reviewed changes

Changed how default value is set for cursor blink speed spinbox

08829f0

ccordoba12 reviewed Aug 1, 2017

View reviewed changes

jnsebgosselin added 2 commits August 1, 2017 12:53

Removed a print statement that was not deleted before push...

4f112ab

removed spaces around = for keyword args and changed step of spinbox

76e3691

goanpeca reviewed Aug 1, 2017

View reviewed changes

ccordoba12 changed the title ~~Add an option to set cursor blinking speed in ms~~ PR: Add an option to set cursor blinking speed in ms Aug 16, 2017

ccordoba12 requested a review from andfoy August 25, 2017 22:01

ccordoba12 changed the title ~~PR: Add an option to set cursor blinking speed in ms~~ PR: Add an option to set cursor blinking speed in miliseconds Aug 26, 2017

ccordoba12 merged commit 13cc5ab into spyder-ide:3.x Aug 26, 2017

ccordoba12 added a commit that referenced this pull request Aug 26, 2017

Merge from 3.x: PR #4758

7fee869

Fixes #4277

jnsebgosselin deleted the caret_blinking_option branch August 29, 2017 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Add an option to set cursor blinking speed in miliseconds #4758

PR: Add an option to set cursor blinking speed in miliseconds #4758

jnsebgosselin commented Jul 18, 2017 •

edited

Loading

goanpeca commented Jul 18, 2017

goanpeca commented Jul 18, 2017

jnsebgosselin commented Jul 18, 2017

goanpeca commented Jul 18, 2017

ccordoba12 commented Jul 18, 2017

jnsebgosselin commented Jul 18, 2017

ccordoba12 Jul 30, 2017

ccordoba12 Jul 30, 2017

ccordoba12 commented Jul 30, 2017

rlaverde commented Jul 31, 2017 •

edited

Loading

jnsebgosselin commented Jul 31, 2017

goanpeca Jul 31, 2017

jnsebgosselin Jul 31, 2017

jnsebgosselin Aug 1, 2017

ccordoba12 Aug 1, 2017

goanpeca commented Aug 1, 2017

ccordoba12 Aug 1, 2017

ccordoba12 Aug 1, 2017

goanpeca Aug 1, 2017

jnsebgosselin Aug 1, 2017

ccordoba12 Aug 1, 2017 •

edited

Loading

ccordoba12 Aug 1, 2017

goanpeca Aug 1, 2017

andfoy commented Aug 25, 2017

ccordoba12 commented Aug 26, 2017

PR: Add an option to set cursor blinking speed in miliseconds #4758

PR: Add an option to set cursor blinking speed in miliseconds #4758

Conversation

jnsebgosselin commented Jul 18, 2017 • edited Loading

goanpeca commented Jul 18, 2017

goanpeca commented Jul 18, 2017

jnsebgosselin commented Jul 18, 2017

goanpeca commented Jul 18, 2017

ccordoba12 commented Jul 18, 2017

jnsebgosselin commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 commented Jul 30, 2017

rlaverde commented Jul 31, 2017 • edited Loading

jnsebgosselin commented Jul 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andfoy commented Aug 25, 2017

ccordoba12 commented Aug 26, 2017

jnsebgosselin commented Jul 18, 2017 •

edited

Loading

rlaverde commented Jul 31, 2017 •

edited

Loading

ccordoba12 Aug 1, 2017 •

edited

Loading