[StyledEdit] increases window number even if previous one was closed

[diver]

This is hrev44133, gcc2hybrid.

• Launch StyledEdit
• Create new window Alt+n
• Close it Alt+w
• Create another new window Alt+n

Notice "Untiled 3" in it's tab title.

[leavengood]

I looked into the code and thought about how to fix this, and there isn't really a nice way to fix it.

The StyledEdit app maintains an integer variable of the untitled count, and increments it each time a new untitled window is created. Let's say we try to fix this: the Untitled 2 window is closed, it informs the StyledEdit app that it was in a state of "untitled", so the app decrements the count (to 1, assuming Untitled 1 is still open.) If another Untitled window is opened, it too will be Untitled 2. Great, that sounds good. Except if there are 3 untitled windows, and you close the second one (bringing the count to 2), when a new window is opened it will be called Untitled 3. But there already is an Untitled 3, so now there are two such windows. Confusing.

The only "proper" way to fix it is with some sort of hashtable mapping of "recyclable" untitled numbers which can be reused, but I don't think it is worth the trouble.

[X512]

Solution can be simple: just find title with mimimum number that don't already used. Since practically no more than 100 windows can be used, windows will be opened fast.

[leavengood]

Hmmm, you have a point. Iterating over the windows wouldn't be all that terrible. I'll think about it.

[diver]

Terminal manages to do that: ​http://cgit.haiku-os.org/haiku/tree/src/apps/terminal/TermWindow.cpp#n191

[bonefish]

Replying to diver:

Terminal manages to do that: ​http://cgit.haiku-os.org/haiku/tree/src/apps/terminal/TermWindow.cpp#n191

It's even more complicated in Terminal, since it is multi-launch. StyledEdit can indeed just iterate over its windows. Or a simple allocator with free list could be implemented.

[leavengood]

OK, I'll bang this out when I get a free moment today.

[leavengood]

Wow, was it 2 months ago when I last worked on this? I did try to "bang something out" back then, but it was not so easy.

Tonight I decided to take a step back and implemented a prototype for a NewWindowPolicy class in Ruby, and I have something working fairly well. The basic idea is to have a simple WindowState class which has a title (BString) and location (BPoint), and the NewWindowPolicy class has methods to get the next WindowState and a method which is called when a window is closed. The next method stores the returned window state in a list. The closed method will check if the closing window's attributes (title and position) is in the list, and if it is, it will be moved from the main list into a recycled list. Then if there is anything in the recycled list it will be used before generating something new in next. I wrote an RSpec for my Ruby class and it works pretty well.

So now I'll try to convert it to C++ and see how that works. Since I recall seeing other bugs about the window handling of other applications in Haiku, I imagine this class (which I'll put in the shared kit) can be used in other places.

[siarzhuk]

This task was published as GCI 2012 task ​http://google-melange.appspot.com/gci/task/view/google/gci2012/7968215

[siarzhuk]

This issue was not claimed during GCI so I reassign it back. IMO Ryan's NewWindowPolicy class idea is the best solution we can invent here.

[siarzhuk]

Replying to ejno1:

Patch. Selects the lowest unused number for new Untitled documents.

Note that as it was discussed in comment:9 we are planning to use more generic solution for this problem that can be utilized in every application requiring such functionality. Could you try to improve your patch in this direction? Thank you for the contribution!

PS: there are some codestyle issues in your patch so please take care about it too. ;-)

[ejno1]

Replying to siarzhuk:

Replying to ejno1:

Patch. Selects the lowest unused number for new Untitled documents.

Note that as it was discussed in comment:9 we are planning to use more generic solution for this problem that can be utilized in every application requiring such functionality. Could you try to improve your patch in this direction? Thank you for the contribution!

PS: there are some codestyle issues in your patch so please take care about it too. ;-)

Sorry, I didn't use that method because I wasn't sure I understood it. It seems to me that WindowState would store the title and position of each window, but I don't see why that's necessary when be_app already contains that information. I'm probably missing something.

I've noticed a slightly embarrassing problem with my code so I'll update it now even if it isn't the final solution.

[ejno1]

Patch to select the lowest unused number for Untitled documents

[ejno1]

The WindowPolicy/class approach

[axeld]

Thanks for your patch! There are a number of issues, though, most of the are coding style related:

• a BWindow runs in its own thread, but you don't do any locking when accessing your list.
• OnOpen() is not a good name, as it doesn't make a pair with OnQuit(). Furthermore, those names don't feel like Be API. I would probably just go with AddWindow(), and RemoveWindow() instead.
• Not sure if it makes sense to keep the class implementation in a header; this might be problematic with regards to localization (ie. instead of doing it automatically, the class depends on strings of the file that includes it).
• a static member variable gets an 's' prefix.
• '{' go to the end of the line.
• Variable names like 't' and 'd' should be avoided.

To get a better idea about our coding style, please have a look at ​https://www.haiku-os.org/development/coding-guidelines.

[ejno1]

Thanks for the feedback, I'll try to fix it soon.

Replying to axeld:

Thanks for your patch! There are a number of issues, though, most of the are coding style related:

• a BWindow runs in its own thread, but you don't do any locking when accessing your list.
• OnOpen() is not a good name, as it doesn't make a pair with OnQuit(). Furthermore, those names don't feel like Be API. I would probably just go with AddWindow(), and RemoveWindow() instead.
• Not sure if it makes sense to keep the class implementation in a header; this might be problematic with regards to localization (ie. instead of doing it automatically, the class depends on strings of the file that includes it).
• a static member variable gets an 's' prefix.
• '{' go to the end of the line.
• Variable names like 't' and 'd' should be avoided.

To get a better idea about our coding style, please have a look at ​https://www.haiku-os.org/development/coding-guidelines.

[diver]

Still here in hrev53129. Should the patch be moved to Gerrit?

[leavengood]

For what it is worth 7 years later I think my NewWindowPolicy idea is a bit overkill and the first patch by ejno1 which iterates over the windows is fine. I will test that patch and get it pushed up to Gerrit when I get a chance.

Sorry for the insane delay on a simple issue...

[X512]

Patch to select the lowest unused number for Untitled documents

I uploaded this patch to Gerrit with minor modifications: ​https://review.haiku-os.org/c/haiku/+/2273. Operation is confirmed.