Opened 7 years ago

Last modified 4 months ago

#8556 assigned bug

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

Reported by: diver Owned by: leavengood
Priority: normal Milestone: R1
Component: Applications/StyledEdit Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

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.

Attachments (2)

untitled_number.patch (1.8 KB ) - added by ejno1 6 years ago.
Patch to select the lowest unused number for Untitled documents
untitled_number_v2.patch (5.0 KB ) - added by ejno1 6 years ago.
The WindowPolicy/class approach

Download all attachments as: .zip

Change History (20)

comment:1 by leavengood, 7 years ago

Resolution: invalid
Status: newclosed

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.

comment:2 by X512, 7 years ago

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.

comment:3 by leavengood, 7 years ago

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

in reply to:  4 comment:5 by bonefish, 7 years ago

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.

comment:6 by leavengood, 7 years ago

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

comment:7 by axeld, 7 years ago

Resolution: invalid
Status: closedreopened

comment:8 by axeld, 7 years ago

Owner: changed from korli to leavengood
Status: reopenedassigned

comment:9 by leavengood, 7 years ago

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.

comment:10 by siarzhuk, 7 years ago

Owner: changed from leavengood to siarzhuk

comment:11 by siarzhuk, 7 years ago

Owner: changed from siarzhuk to leavengood

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.

comment:12 by ejno1, 6 years ago

Has a Patch: set

in reply to:  12 ; comment:13 by siarzhuk, 6 years ago

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. ;-)

in reply to:  13 comment:14 by ejno1, 6 years ago

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.

by ejno1, 6 years ago

Attachment: untitled_number.patch added

Patch to select the lowest unused number for Untitled documents

by ejno1, 6 years ago

Attachment: untitled_number_v2.patch added

The WindowPolicy/class approach

comment:15 by axeld, 6 years ago

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.

in reply to:  15 comment:16 by ejno1, 6 years ago

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.

comment:17 by diver, 4 months ago

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

comment:18 by leavengood, 4 months ago

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...

Note: See TracTickets for help on using tickets.