Skip to content
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

Completly breaking ST3 process and GUI #15

Closed
Heraes-git opened this issue Sep 2, 2019 · 15 comments
Closed

Completly breaking ST3 process and GUI #15

Heraes-git opened this issue Sep 2, 2019 · 15 comments

Comments

@Heraes-git
Copy link

Heraes-git commented Sep 2, 2019

After installation, I opened the SublimeSetWindowTitle's settings, and added this to my user :

{
     "template": "Pages",
}

, to rename the actual window with the workspace/project's name (I had several opened, but I supposed that because I had called the plugin's settings from one of them, it would assign the name on it).

Unfortunately for me, the shit started to hit the fan : everything broke up, and some strange error window appeared, followed by the ST UI being stuck and the process frozen.

2019-09-02_23h03m55s_sublime_text

Then it was a hard time for me to figure out how to end the nightmare, because I could not exit the workspace by hitting the close button (was frozen), and I had to kill the ST process tree ; but each time I reopened ST, the package tried again to rename the window(s), and the cycle started again.

So at a moment, I managed to reach the Control Palette for removing the plugin, but it was freezing the UI again. I tried to delete the package's file directly, but unsuccessfully given that it reinstalled it automatically at ST's reopening (strange virus feeling).

After several attempts, I succeed in removing the package in the Control Palette and closed all the windows pseudo-correctly (one had the theme's broken, and completly white !).

So please do something to block downloading that package until you fix it. It could lead to drama for some hurry developers that haven't my patience.

PS : Btw I'm on Windows 10, with ST portable v3.2.1 build 3207.

@gwenzek
Copy link
Owner

gwenzek commented Sep 3, 2019

Hi @Romarain , I'm very sorry for what happened. I hope you didn't lose too much time.

This plugin is making use of Windows API which uses string pointer and is generally "unsafe" in the sense that it can segfault if wrongly used.
I personally haven't implemented the Windows version (this was a Linux only package at the start) but it has been reported working correctly on Win 7 and Win 10 (see #10).

@robertcollier4 and @bahanonu could you try to use "template": "Pages" on Windows ? Save all your work before since it may crash ST.

but unsuccessfully given that it reinstalled it automatically at ST's reopening (strange virus feeling).

That's unfortunately how Package Control works. You have a User/Package Control.sublime-settings on your Packages folder, which contains the list of packages you want to install. While SetWindowTitle is listed it will be downloaded and installed by Package Control. In case of crash you need to edit the file with another program, and remove the SetWindowTitle from the list as well as deleting the SetWindowTitle folder like you did.

@gwenzek
Copy link
Owner

gwenzek commented Sep 3, 2019

Also could you tell me if it worked before changing the settings ?

Also the error message seems related to the Compare Side By Side package. Could you try with a brand new portable ST, with only SublimeSetWindowTitle package ?

@Heraes-git
Copy link
Author

Hi,
It's okay, it took only 10 looong minutes, and it ended well.
Before changing the settings, it seemed that the titlebar was displaying what I saw in the template option (the " - ST" at the end).
I'm tired today (bad sleep) so I'm gonna test tomorrow.

PS : do you confirm that we can use your plugin several times, to rename several opened windows ?

@bahanonu
Copy link

bahanonu commented Sep 3, 2019

@gwenzek @Romarain

Tested three SetWindowTitle scenarios all on Windows 10 Pro 64 bit with Sublime build 3207.

  • with a build that looks to have been downloaded on (6 or 12) Oct 2018.
  • build re-downloaded via Package Control with the my existing Sublime (several packages installed).
  • build re-downloaded via Package Control with a new Windows 64 bit portable version.

When I changed my entire set_window_title.sublime-settings to

{
  "template": "Pages"
}
  • Old SetWindowTitle build: it works as expected and Sublime window titles will just display Pages instead of other information.
  • With the current Package Control SetWindowTitle build (either with existing or new Sublime installation): it will run fine on booting but if I try to change set_window_title.sublime-settings while Sublime is running, it crashes and I have to restart (have everything in projects/workspaces, so not losing any work); this does not occur on the old build.

I have attached the old working Windows 10 build below if you want to check the diff to see where things might have changed leading to the issue.

SetWindowTitle_20181006.zip

Can take a look at this again sometime this week.

@gwenzek
Copy link
Owner

gwenzek commented Sep 3, 2019

I'd bet on this part of the code, https://github.com/gwenzek/SublimeSetWindowTitle/pull/10/files#r320254055
But I can't test it. And it's not clear why we wouldn't find anything since we are searching by class.

@gwenzek
Copy link
Owner

gwenzek commented Sep 3, 2019

@Romarain

do you confirm that we can use your plugin several times, to rename several opened windows ?

With the default configuration you will have each windows named ({project}) {filename} - ST. So each window will have a different name, but I think you can only have one pattern.

@robertcollier4
Copy link
Contributor

Tested on WinXP-32bit and Win7-32bit with multiple open windows to have the same name - it is working fine with "template":"Pages" for me. Can someone try on Windows 10 the following change recommended on PR #10:

  1. Add "if(hwndSublime):" below the line "hwndSublime = ctypes.windll.user32.FindWindowA(b'PX_WINDOW_CLASS', None)"
  2. Indent the following line.

Test screen_ capture with Licecap of current version working fine in Win7-32bit:
2019-06-10_1757

@robertcollier4
Copy link
Contributor

So each window will have a different name, but I think you can only have one pattern.

There is no such rule in Windows that different windows cannot have the same Title. Windows are referenced by ID, not by Class or Title (both which can be duplicate).

Also very odd that this is causing a SublimeText error of loading colour scheme which is totally unrelated so I suggest we keep looking.

@bahanonu
Copy link

bahanonu commented Sep 3, 2019

Tested on WinXP-32bit and Win7-32bit with multiple open windows to have the same name - it is working fine with "template":"Pages" for me. Can someone try on Windows 10 the following change recommended on PR #10:

  1. Add "if(hwndSublime):" below the line "hwndSublime = ctypes.windll.user32.FindWindowA(b'PX_WINDOW_CLASS', None)"
  2. Indent the following line.

This works fine on a fresh Windows 10, Sublime Build 3207 on my end with issues noted below.

Sublime will crash if I edit set_window_title.sublime-settings using the same Sublime instance that the settings are supposed to be applied to or when using Notepad. If I use a different Sublime instance to change the settings file, I can do so while the main Sublime is running and it will change the window title without issue.

The above behavior does not occur with the SetWindowTitle build I attached in #15 (comment). I checked by swapping the SetWindowTitle package directories in the same fresh Sublime install and using that Sublime instance to edit the title multiple times without crash or issue.

@bahanonu
Copy link

bahanonu commented Sep 3, 2019

@gwenzek @robertcollier4

I took a look at the set_window_title.py diff between the old and new SetWindowTitle builds and it appears the main section that was added is here:
image

This seemed like the most obvious area causing the crash based on what this section of code is doing. I removed the below lines (see https://github.com/gwenzek/SublimeSetWindowTitle/blob/master/set_window_title.py#L44-L57) and now the plugin works as expected when changing the settings with the same/different Sublime instance or Notepad (Windows 10, Sublime build 3207) except it will only change after switching focus or changing tabs.

  # Update all window titles on setting change.
  settings = sublime.load_settings("set_window_title.sublime-settings")
  setting_keys = [
    "unregistered",
    "template",
    "has_project_true",
    "has_project_false",
    "is_dirty_true",
    "is_dirty_false",
    "path_display",
    "untitled",
  ]
  for k in setting_keys:
    settings.add_on_change(k, refresh_all)

@robertcollier4
Copy link
Contributor

There is a existing note that "refresh_all" is "Only enabled on Linux because for some reason it freezes ST on Windows.". So this is a existing bug not one introduced by the recent commit. We should not be running refresh_all on Windows then. The solution is to add "if PLATFORM == "linux":" above the code that bahanou has posted in the post just above this.

Then "refresh_all" will not run on Windows on changing settings while SublimeText is open.

@robertcollier4
Copy link
Contributor

Or we might not need that "add_on_change" snippet that bahanou posted at all. Is there any reason that is there? I cant even find any documentation on "add_on_change" and it seems unnecessary. Perhaps we should remove the entire section bahanou has posted since "add_on_change" is unnecessary. The plugin reloads itself just fine.

@robertcollier4
Copy link
Contributor

Also, "refresh_all" is being called 8 times - once for every item in the array settings_key - this is completely unnecessary. Lets disable refresh_all from being called in Windows at all as per the note and if we want to keep it we can have it called only once on Linux.

@gwenzek
Copy link
Owner

gwenzek commented Sep 4, 2019

the add_on_change allow to update the window tItle when one of the settings changes. If you're directly editing the plugins settings file then the plugin is reloaded anyway, but that's not the case if you're overriding the settings using a file in your Packages/User folder.

But it's true that it looks like refresh_all shouldn't be called at all on Windows.
Maybe the "on_change" should only be registered for Linux.

gwenzek pushed a commit that referenced this issue Nov 8, 2019
* Remove repeated calls to refresh_all in Windows on settings change. Fixes Issue #15

* On Windows update window title on plugin_loaded for top window

* add_on_change for Linux only

* Remove update window title on plugin_loaded in Windows due to freezes
@gwenzek
Copy link
Owner

gwenzek commented Nov 8, 2019

This should be fixed by #16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants