Bluetooth UI layer refactoring and improvement

[hamish]

This is a ticket to track some UI work I'm doing on Bluetooth:

Refactoring of the Bluetooth output window code.

Improvement of the preflet's use of the layout manager.

[yourpalal]

Assigning myself as owner, as I was already working with Hamish for GCI.

[hamish]

Removes superfluous code from Bluetooth's output window

[yourpalal]

Hi hamish, this patch looks good, but there are a 2 things I would change:

• Add a private OutputView* Output::_OutputViewAt(int32 index) method, which gets the view from the tab at that index, then casts and returns it. This will make things a little prettier too.
• I don't see the need for fOutputViewsList either, it's now only used in one place, and you can use the above method to replace it.

Good catch on the 80-column limit as well. Thanks!

-Alex

[hamish]

I believe fOutputViewsList is still needed. When the other Bluetooth modules add tabs to the output window, they supply their own arbitrary constant as an identifier for the tab, and this constant doesn't neccessarily match up to the view's index on the tab view, so it needs to be stored.

[yourpalal]

Replying to hamish:

I believe fOutputViewsList is still needed. When the other Bluetooth modules add tabs to the output window, they supply their own arbitrary constant as an identifier for the tab, and this constant doesn't neccessarily match up to the view's index on the tab view, so it needs to be stored.

Yes, actually, I just looked around a bit and I think you are right. Its sort of a strange way of doing things, but alas. So, fOutputViewsList can stay, but I would still like to see the _OutputViewAt() method used in Output::MessageReceived(). I think though, in light of this new info, _OutputViewAt() should be call _OutputViewForTab(), or something similar.

[hamish]

Removes superfluous code from Bluetooth's output window

[hamish]

Yep, looks nicer now. Also underscored the other private method.

I was also looking at the Bluetooth preflet with a view to tidying up its UI, but I think it's too unfinished to start messing with; there are a lot of TODOs. Removing the inset from the menu bar would be the only thing for me to do.

[yourpalal]

Replying to hamish:

Yep, looks nicer now. Also underscored the other private method.

Yes, your new patch looks good to me, I'm away from Haiku at the moment, but barring any compilation errors/run-time glitches (I don't expect to see any), I will be able to commit this soon. Thanks for your work!

I was also looking at the Bluetooth preflet with a view to tidying up its UI, but I think it's too unfinished to start messing with; there are a lot of TODOs. Removing the inset from the menu bar would be the only thing for me to do.

Along with the menubar not being flush to the window there seems to be some problems with button positioning which you could possibly look into. I haven't looked at that code yet, so that may be related to the TODOs you mention.

[oruizdorantes]

Replying to hamish:

I believe fOutputViewsList is still needed. When the other Bluetooth modules add tabs to the output window, they supply their own arbitrary constant as an identifier for the tab, and this constant doesn't neccessarily match up to the view's index on the tab view, so it needs to be stored.

Well we could try to change that so that Adding a TAB the class returns the assigned index or something like that. Will make things less simple in the server side, having to store all the assigned numbers instead of the fixed constants

[oruizdorantes]

I understand that the proposed _OutputViewAt() will use retrieve the BTab with the index and then use BTab()::View() to retrieve the View and then cast it?

Regarding the preflet it is a lot of work undone, but as far as I remember regarding UI alignment there should be no problem fine.

As well there is some class refactoring which should be moved to the bluetooth kit(libbluetooth.so) that could be handled in a separate ticket, and I could describe the first actions required.

[yourpalal]

Replying to oruizdorantes:

Replying to hamish:

I believe fOutputViewsList is still needed. When the other Bluetooth modules add tabs to the output window, they supply their own arbitrary constant as an identifier for the tab, and this constant doesn't neccessarily match up to the view's index on the tab view, so it needs to be stored.

Well we could try to change that so that Adding a TAB the class returns the assigned index or something like that. Will make things less simple in the server side, having to store all the assigned numbers instead of the fixed constants

Another option is to pass the view name in place of the index, which combines the best of both worlds (fixed "handles" + no extra BList). This also makes it simple to find the view in question using BView::FindView(). All that would be needed on the server side would be to make sure the string literals are assigned to a const char* somewhere. How does that sound to you?

[oruizdorantes]

Another option is to pass the view name in place of the index, which combines the best of both worlds (fixed "handles" + no extra BList). This also makes it simple to find the view in question using BView::FindView(). All that would be needed on the server side would be to make sure the string literals are assigned to a const char* somewhere. How does that sound to you?

I like the idea, but I guess we would have an overhead each time we post debug information for looking for the view, comparing strings :-/

[yourpalal]

Replying to oruizdorantes:

Another option is to pass the view name in place of the index, which combines the best of both worlds (fixed "handles" + no extra BList). This also makes it simple to find the view in question using BView::FindView(). All that would be needed on the server side would be to make sure the string literals are assigned to a const char* somewhere. How does that sound to you?

I like the idea, but I guess we would have an overhead each time we post debug information for looking for the view, comparing strings :-/

Yes, but the overhead will still be small, considering the small number of strings to compare, and the likelihood of 'short-circuiting' most comparisons (e.g. they fail on the first or second character). It will of course be slower than a constant time operation (as we have now).

[hamish]

I took a look at implementing the view-fetching using view names only, but saw that the server creates "Local Device" tabs for each new device, using the device ID as the index with which to access the view. If we were to convert this functionality to use view names, the device IDs would have to be sprintf'd into a string every time we want to look it up...

I feel that the BList currently in use is preferable to that option.

[yourpalal]

Replying to hamish:

I took a look at implementing the view-fetching using view names only, but saw that the server creates "Local Device" tabs for each new device, using the device ID as the index with which to access the view. If we were to convert this functionality to use view names, the device IDs would have to be sprintf'd into a string every time we want to look it up...

I feel that the BList currently in use is preferable to that option.

Okay, sounds good. That means your patch is finished as-is, so I will commit it tonight or tomorrow. Feel free to start working on another ticket in the mean-time (if you want). Thanks for you work!

[yourpalal]

Hi Hamish, I committed your patch in hrev40237, but I see that you also mentioned doing the layout work in the bluetooth preflet on this ticket, so if you want to do that here, I will leave this ticket open, otherwise I will close it as 'fixed'.

[yourpalal]

Okay, I am closing this ticket as fixed now.