Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#7445 closed enhancement (fixed)

app_server DecorManager + DecorInfo patch

Reported by: looncraz Owned by: axeld
Priority: normal Milestone: Unscheduled
Component: Servers/app_server Version: R1/Development
Keywords: DecorManager, decorator Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

Multiple changes in this one, all very tight knit so no real way to separate them, sorry.

  1. app_server: DecorManager: reduce responsibilities, remove DecorAddOn list
  1. Implement DecorInfo in interface ( libbe.so )
  1. Implement DecorInfoUtility to attain any and all information related to decorators
  1. Make changes to InterfaceDefs.cpp private functions to suit reduced server-side responsibilities
  1. Implement live previewing as part of modifications.
  1. Attach descriptive resources to each decorator to satiate DecorInfo
  1. Modify setdecor utility to use DecorInfoUtility, including preview.
  1. Modify Appearance Preflet to compile with DecorInfoUtility in preparation for further changes.

These modifications are designed to overcome the following short-falls in the current design:

  1. DecorManager scanned and loaded every decorator on boot
  2. DecorManager held said decorator in memory -forever-
  3. Newly installed or changed decorators could not be used without restart
  4. server-side list management, once fully added, would be inflexible

Attached, please find the patch in question.

Else: http://looncraz.txnj.net/app_server-DecorManager-DecorInfo.patch

Warning: This patch has been tested, but may not be perfect.

Attachments (2)

app_server-DecorManager-DecorInfo.patch (57.2 KB ) - added by looncraz 8 years ago.
app_server-DecorManager-DecorInfo.patch
cleanups.diff (43.8 KB ) - added by stippi 8 years ago.
Diff of the patch before and after coding style cleanups and other improvements.

Download all attachments as: .zip

Change History (31)

comment:1 by looncraz, 9 years ago

Has a Patch: set

comment:2 by stippi, 9 years ago

Thanks for the patch and also for using the proper channels! :-) The changes are definitely fine and welcome. If you feel motivated enough, you could do another iteration of coding style cleanup, otherwise I would do it before applying the patch. The biggest inconsistencies are with parenthesis indentation in the files you wrote from scratch. Some more specific remarks which stuck from reading the patch:

  • Please keep the asterisk style consistent (DecorManager.h and others). We prefer "Type* name". In a header, the asterisk would go by the type as well, with tabbing until the name.
  • Please keep opening parenthesis indentation consistent. The opening one goes on the same line, except for funtions and blocks. In the files written from scratch, you have both inconsistent indentation and put the opening one on a new line...
  • Please don't change the copyright style. So instead of doing this:
 /*
- * Copyright 2007, François Revol, revol@free.fr. 
  * Distributed under the terms of the MIT license. 
  * 
+ *  Authors: 
+ *      François Revol <revol@free.fr> 
+ *      Joseph "looncraz" Groover <looncraz@satx.rr.com> 
+ */

(which BTW removes the actual Copyright line), you should do this:

 /*
+ * Copyright 2011, Joseph "looncraz" Groover <looncraz@satx.rr.com>
  * Copyright 2007, François Revol, revol@free.fr. 
  * Distributed under the terms of the MIT license. 
  */
  • Other than the coding style violations, the functionality seems fine to me, but I have only skimmed over a lot of chunks of the patch. Your solution with the preview window is certainly "creative" :-). I would personally have liked it better, if a decorator could render into a bitmap and that would be displayed in Appearance. Each decorator side by side, like in the GNOME appearance panel. So the user just clicks on the one he thinks looks good.
Version 0, edited 9 years ago by stippi (next)

comment:3 by looncraz, 9 years ago

I will go through and make the changes suggested tonight/tomorrow and submit a revised patch.

I will also do some more bug testing for an edge case I missed and check the rescan functionality to see if there are any problems in that arena.

Look for an update tomorrow!

Thanks Stippi!

Also, one note, I have had conflicting reports in regards to the placement of the pointer indicator (*). I, by nature, use Type * pointer, * pointer2; However I was reprimanded and told to put the asterisk with the variable, which is more correct to me than with the Type.

Or maybe y'all are saying different things...

As in you are saying:

void Function(Type* pointer);

and he is saying

function... {

Type *pointer...

}

One in the body, the other in the header... different rules in different areas ( makes sense to me that way ).

I've read the guidelines, but I suppose I'll need to read them again.

I heavily modified my code to fit the code base, though I can see I missed plenty.

Anyway... tomorrow will bring a patch... to the patch... :-)

--The loon

EDIT: One quick addition:

In regards to the preview method, I was originally planning on rendering to a bitmap until I realized how much better it would be to have a truly live preview. You click on a name and the Appearance window itself changes to the selected decor as an automatic preview. Or, a child window would cycle through the various looks of a given decor, showing any fancy features at the same time ( such as Stack & Tile, Sliding tabs, or my up-and-coming well.. we'll get there :-) ).

Just my thoughts. I plan to implement children windows, non-square windows, and alpha-blending of select windows / decors ( we'll see on that last one.. it'll be a challenge ) in the future, so I'm kinda thinking ahead :-)

Thanks again!

Last edited 9 years ago by looncraz (previous) (diff)

comment:4 by axeld, 9 years ago

When in doubt, there is always the http://www.haiku-os.org/development/coding-guidelines page - while it surprisingly does not mention the asterisk style as far as I can tell, the right one (ie. "Type* variable") is used in all examples. And this is also what Michael and Stippi told you.

in reply to:  4 comment:5 by looncraz, 9 years ago

Replying to axeld:

When in doubt, there is always the http://www.haiku-os.org/development/coding-guidelines page - while it surprisingly does not mention the asterisk style as far as I can tell, the right one (ie. "Type* variable") is used in all examples. And this is also what Michael and Stippi told you.

Well I'll be... Your comment spurned me into re-reading what Micheal sent.. and you're right, they did both say the exact same thing! HAHA!!

Wow... brains are funny!

Okay, I'll be making those changes now ( I've been coding on projects for the last 10 years where the asterisk belonged to the variable ( which makes sense ) ).

That, and my natural writing style is space-heavy. I like the space-bar, I can hit it with TWO fingers.. so it must be important!

Stay tuned...

--The loon

in reply to:  4 comment:6 by looncraz, 9 years ago

I have made all changes requested ( I think ) and corrected an issue I found after writing a stress test ( in DecorManager::PreviewDecorator ).

The attached patch has been updated and should be ready for application without modification.

Thank you!

in reply to:  2 comment:7 by looncraz, 9 years ago

I made the changes and submitted an updated version of the patch some days ago but haven't seen any further activity so I 'd figured I'd ping you to see if you noticed.

Axel is admittedly hard to reach at times :-/

The patch should be acceptable without changes, and survives stress tests - though I will likely modify some portions in the future ( namely DecorInfoUtility::ScanDecorators() ).

Thanks!

comment:8 by michaelvoliveira, 8 years ago

Glad to see you contributing with Haiku, looncraz

please keep the good work

comment:9 by scottmc, 8 years ago

Milestone: R1Unscheduled

Pushing this out to post-R1 as it is feature creep. If a developer has time to check it over and apply before then, then that's fine, but it shouldn't hold up R1 either.

comment:10 by stargatefan, 8 years ago

I found 1 small typo in the patch although it could have been cuased by patch itself. Very simple fix. , the patch works great. also seems to lower the ram footprint a tiny bit and makes the ui noticeably snappier on a livecd. All it needs is a gui for the decorators and in the preflets and its a functional addition to haiku, I am going to install and look for regressions but there no reason to stick this patch that far back in the cue.

Last edited 8 years ago by stargatefan (previous) (diff)

in reply to:  10 ; comment:11 by looncraz, 8 years ago

Replying to stargatefan:

I found 1 small typo in the patch although it could have been cuased by patch itself. Very simple fix. , the patch works great. also seems to lower the ram footprint a tiny bit and makes the ui noticeably snappier on a livecd. All it needs is a gui for the decorators and in the preflets and its a functional addition to haiku, I am going to install and look for regressions but there no reason to stick this patch that far back in the cue.

Thank you for taking a look!

Funny thing is that the entire time I thought about the patch I was thinking from more of a purity stand-point in regards to division of responsibilities while wholly overlooking the likelihood of a performance increase.

Makes sense now that I think about it. No more list look-ups, removed several copies during the instantiation process ( just in making the code cleaner and more uniform ).

Let me know if there are any issues, I would like to augment the preview capabilities as a separate patch, and maybe I'll take a break from LoonCAFE and jump back over to make some modifications to the Appearance preflet.

Thanks again!

--The loon

comment:12 by stippi, 8 years ago

Could you please point out where there have been list lookups replaced that are performance relevant to make the "UI noticeably snappier"? That statement looks really dubious to me. I understand the desire to get patches in, it contains hard work, but nobody needs to resort to such claims (but I'll apologize when you can indeed point out the performance improvements in the code). The real reason that I have not applied this patch yet is not that I need more incentive to do so, but because there are still a whole lot of coding style violations. Before applying the patch, I intend to fix them, and it's just a daunting work, I have no time and I just don't understand why I have to do it and not looncraz. I also pointed out that the preview solution is pretty hacky, but I understand that the motivation is low to code a proper solution (which would be to let Decorators render into a bitmap and show the user a preview of all decorators side by side before he even selects any other).

in reply to:  11 comment:13 by stippi, 8 years ago

Replying to looncraz:

I would like to augment the preview capabilities as a separate patch, and maybe I'll take a break from LoonCAFE and jump back over to make some modifications to the Appearance preflet.

Yes, providing another solution for the preview feature would be very welcome, but to provide more incentive to apply the patch as is, it would just need another round of coding style cleanup. Maybe a lot of the violations are caused by your editor not displaying them. DecorInfo.cpp (just an example) has a whole lot of really weird spacing. Maybe just look at the patch in Trac here, if you don't see what I mean in your editor. The variable naming is not good (our style guide has a whole lot to say about good variable naming, the naming style, where to declare variables (hint: not at the top of functions)). The asterisk style is still inconsistent as well.

I appreciate your work, I really do, and if you don't have time or even just no motivation to fix the coding style issues, I fully understand. But the reason I didn't apply the patch is simply because I have no time to fix those issues either.

in reply to:  12 ; comment:14 by stargatefan, 8 years ago

Replying to stippi:

Could you please point out where there have been list lookups replaced that are performance relevant to make the "UI noticeably snappier"? That statement looks really dubious to me. I understand the desire to get patches in, it contains hard work, but nobody needs to resort to such claims (but I'll apologize when you can indeed point out the performance improvements in the code). The real reason that I have not applied this patch yet is not that I need more incentive to do so, but because there are still a whole lot of coding style violations. Before applying the patch, I intend to fix them, and it's just a daunting work, I have no time and I just don't understand why I have to do it and not looncraz. I also pointed out that the preview solution is pretty hacky, but I understand that the motivation is low to code a proper solution (which would be to let Decorators render into a bitmap and show the user a preview of all decorators side by side before he even selects any other).

I noticed that the patch made the UI snappier and a bit more fluid. I didn't go in expecting anything in that regard I was looking for regressions etc that could be show stoppers.but it was noticeable enough that I noticed it, I also saw a .5% drop of cpu resource at idle use on the anyboot with this patch over the current revision nightlys. Yes I double checked.

It was fiarly noticeable on the anyboot cd. There is a bug with enabling stack and tile. It simply won't enable from terminal commands. and I think its the naming scheme or some inconsistency, I will take a look.

The typo in the patch is decorinfo.cpp line 276 IIRC.

Configuration preflet would be much appreciated from me and many others I assume. I have a thing againt using terminal to do basic stuff in the OS. I always remeber that if my wife can't click on it, she can't do it.

One thing I did notice Stippi. What ever the total of these changes does, when QT cuases deskbar crashs, and it sometimes does, instead of the desktop deskbar locking up randomly, it is more easily recorvered.

Now if thats a byproduct of this patch or other fixs ?? But I did notice that deskbar seems to more easily recovered with this patch.

That would take a good bit of chasing down to determine

BTW these are my observations, they are not tested facts. But you can always test for yourself and see what that gets ya.I think you are vastly more knowledgable then myself and would be able to test for this.

in reply to:  12 comment:15 by looncraz, 8 years ago

Replying to stippi:

Could you please point out where there have been list lookups replaced that are performance relevant to make the "UI noticeably snappier"? That statement looks really dubious to me. I understand the desire to get patches in, it contains hard work, but nobody needs to resort to such claims (but I'll apologize when you can indeed point out the performance improvements in the code). The real reason that I have not applied this patch yet is not that I need more incentive to do so, but because there are still a whole lot of coding style violations. Before applying the patch, I intend to fix them, and it's just a daunting work, I have no time and I just don't understand why I have to do it and not looncraz. I also pointed out that the preview solution is pretty hacky, but I understand that the motivation is low to code a proper solution (which would be to let Decorators render into a bitmap and show the user a preview of all decorators side by side before he even selects any other).

IIRC, the (sole?) location would be in DecorManager.cpp while looking up which decorator to apply to a window. This is entirely from memory and I code 6/7 days a week on a half dozen or so projects so sometimes things get fuzzy ;-) Correct me if I'm wrong, I'm not making any claims to performance increases or stability improvements, just that I *shouldn't* be causing new issues. Also that a performance increase should be noticed at boot. While I'd like to see the patch applied, I'm in no serious rush. I can always add this patch in manually to get what I need for development. I'm an absolute perfectionist so I would like to resolve any issues myself so that future submissions are more easily acceptable.

Further, since the last comments made by you I did a drastic round of code clean-up. Pe makes the code look perfect in regards to tabbing and spacing, but on here it does look weird. No idea how to address that, sorry. My natural tabbing style only has one conflict with the coding guidelines ( I align in-function - well, everything... ). I removed all I found in a single round of code cleanup, I'll do another run today ( since, I have the time ).

I have no issue in providing what you call a proper preview solution, but I prefer a true live preview. This idea is precisely the method Be used in Dano, because, when carried to the UI, there are many options ( such as changing just the Appearance window's decorator when selecting a decorator from the list ). A bitmap rendering remains static, and is generally unable to demonstrate any features.

However, the ideas should be combined, IMHO. I still need to find out how to do the rendering server-side. My original patch had a static preview, and I got much static from that. One problem is the more you do server-side the more likely you open you are to system crashes, which is something I'd like see avoided. (granted a better debugger could go a long way, I think about the inexperienced user ).

In th eend, the UI development will determine the feature requirements. I believe a static preview attachment of a set size as a resource would be ideal so we can merely put GetPreviewBitmap(...) back into DecorInfoUtility and we have the best of both ideas while permitting additional creative license to decorator designers.

In any event, this preview functionality shouldn't be a hold-up, IMHO, nothing is set in stone until a full UI is developed and released to the public - and then it still is not in stone. I made the changes behind the scenes in regard to BWindow and Window so that no compatibility issues would arise.

--The loon

in reply to:  14 comment:16 by looncraz, 8 years ago

Replying to stargatefan:

Replying to stippi:

I noticed that the patch made the UI snappier and a bit more fluid. I didn't go in expecting anything in that regard I was looking for regressions etc that could be show stoppers.but it was noticeable enough that I noticed it, I also saw a .5% drop of cpu resource at idle use on the anyboot with this patch over the current revision nightlys. Yes I double checked.

I'm not sure how my changes could have reduced static (idle) CPU usage, so I must attribute that elsewhere. Performance on boot ( removed 3/4 disk seeks ) should be slightly improved, and performance when opening a new window should be almost un-noticeably faster. Memory usage should have dropped by as much as half a meg, though ( mostly due to Stack & Tile ).

It was fiarly noticeable on the anyboot cd. There is a bug with enabling stack and tile. It simply won't enable from terminal commands. and I think its the naming scheme or some inconsistency, I will take a look.

Not a bug, I'm assuming your using the ampersand ( & ), which will mess with the command line. You need to escape it (\&) or you can use the shortcut name (SATDecorator). You will notice that setdecor will accept Stack \& Tile, "Stack & Tile" [not sure if you need to escape it in quotes), and SATDecorator. It will accept Window 95, "Window 95", and WinDecorator... and so on...

The command line util will soon be taking the back seat, as it should.

I'll take a look for that type today, thanks for browsing the code!

Now if thats a byproduct of this patch or other fixs ?? But I did notice that deskbar seems to more easily recovered with this patch.

My changes should have no effect here, but it is not impossible if you are testing only the addition of my patch versus the same build without it. I couldn't begin to hypothesize the cause of such behavior, however. I tend to fix any problem I find when I look through code ( I had to restrain myself from re-writing Decorator, as I want it to be fully aware of the window on which it operating for greater feature possibilities - though there are issues potential with that... ).

Thanks for giving it a run, and your input stargatefan!

--The loon

comment:17 by stargatefan, 8 years ago

No problem on the testing, I will keep running the patch but I have 24 hours of up time and no noticeable regressions and I have tried most every application I can think of to stress it.

Seems ok to me

I would fix the stack and tile though

should just be

Setdecor stackandtile IMHO

heres the line I corrected

if (!entry.Exists()) {

fprintf(stderr, "server MIA the world has become its slave! call the CIA\n");

heres how it was and it cuased a compiler error obviously

if (!entry.Exists()) {

fprintf(stderr, "server MIA

the world has become its slave!

call the CIA\n");

I have no idea why its line 267 of the DecorInfo.cpp

Can't say as I noticed a change in boot time, once my machine clears the disk icon it usually jumps to the desktop in moments. Although I didn;t see this change in your patch, is this a possiable bug in haiku or just a weird patching artifact ?

If you come up with a preflet, let me know I will test that to for you.

Last edited 8 years ago by stargatefan (previous) (diff)

by looncraz, 8 years ago

app_server-DecorManager-DecorInfo.patch

comment:18 by looncraz, 8 years ago

I have made the changes as follows:

Strictly enforced the 80 char line limit where it failed previously.

Found several location where the opening stanza was on the next line instead of on the same line of a conditional statement.

Found a single place in all the code where variables were allocated at the opening of a function.

Corrected an odd issue with a multi-line printf statement ( thanks stargatefan! )

Changed the name of Stack & Tile to Stack and Tile.

In addition to those changes I propose related, but not integral, changes to the names of the generated decorators:

WinDecorator = windows95 MacDecorator = macos9 SATDecorator = stackandtile BeDecorator = beos

If this code is accepted and merged upstream I will create a new Appearance preflet using the code and implement either a static preview or a generated preview depending on popular input.

Thanks!

--The loon

Last edited 8 years ago by looncraz (previous) (diff)

in reply to:  18 ; comment:19 by stargatefan, 8 years ago

Replying to looncraz:

I have made the changes as follows:

Strictly enforced the 80 char line limit where it failed previously.

Found several location where the opening stanza was on the next line instead of on the same line of a conditional statement.

Found a single place in all the code where variables were allocated at the opening of a function.

Corrected an odd issue with a multi-line printf statement ( thanks stargatefan! )

Changed the name of Stack & Tile to Stack and Tile.

In addition to those changes I propose related, but not integral, changes to the names of the generated decorators:

WinDecorator = windows95 MacDecorator = macos9 SATDecorator = stackandtile BeDecorator = beos

If this code is accepted and merged upstream I will create a new Appearance preflet using the code and implement either a static preview or a generated preview depending on popular input.

Thanks!

--The loon

Thanx for the fix's. I will update my build and retest. 32 hours up and no problems yet. I even compiled haiku on haiku, no problem "aside from my other ticket about multy core cpu's crashing". aside from that works fine for me.

I am have zero commit rights etc, just another yahoo in the mix. I'd love to see a preflet for setting the decorator.

Thanx for the hard work !

in reply to:  19 ; comment:20 by looncraz, 8 years ago

Thanx for the fix's. I will update my build and retest. 32 hours up and no problems yet. I even compiled haiku on haiku, no problem "aside from my other ticket about multy core cpu's crashing". aside from that works fine for me.

I am have zero commit rights etc, just another yahoo in the mix. I'd love to see a preflet for setting the decorator.

Thanx for the hard work !

Thank you for the interest and the testing!

I have been running the patch since I made it without a single issue, so I'm pretty confident I didn't introduce any issues, glad to have someone else looking it over.

If you want to use setdecor [option] stackandtile just rename the SATDecorator to stackandtile ( or whatever else ).

I will need to look over the original to code to see if I can see anywhere I may have caused unexpected performance improvements. I have to admit it feels a bit snappier to me as well, but I just assumed that was a mind trick ( still could be, naturally ).

The changes I've made should made this patch acceptable, so I doubt there will be much issue getting it in after alpha 3 so I'll go ahead and start some rough GUI concepts using LoonCAFE's UIBuilder to drastically hasten the process.

Thanks again!

--The loon

in reply to:  20 comment:21 by stargatefan, 8 years ago

Replying to looncraz:

Thanx for the fix's. I will update my build and retest. 32 hours up and no problems yet. I even compiled haiku on haiku, no problem "aside from my other ticket about multy core cpu's crashing". aside from that works fine for me.

I am have zero commit rights etc, just another yahoo in the mix. I'd love to see a preflet for setting the decorator.

Thanx for the hard work !

Thank you for the interest and the testing!

I have been running the patch since I made it without a single issue, so I'm pretty confident I didn't introduce any issues, glad to have someone else looking it over.

If you want to use setdecor [option] stackandtile just rename the SATDecorator to stackandtile ( or whatever else ).

I will need to look over the original to code to see if I can see anywhere I may have caused unexpected performance improvements. I have to admit it feels a bit snappier to me as well, but I just assumed that was a mind trick ( still could be, naturally ).

The changes I've made should made this patch acceptable, so I doubt there will be much issue getting it in after alpha 3 so I'll go ahead and start some rough GUI concepts using LoonCAFE's UIBuilder to drastically hasten the process.

Thanks again!

--The loon

I have been learning with this for a while. Makes building UI's much easier IMHO.

Its works pretty good.

http://haikuware.com/directory/view-details/development/ides/niue

Thanx for your hardwork, its great to see new contributors !!

Last edited 8 years ago by stargatefan (previous) (diff)

comment:22 by stippi, 8 years ago

Resolution: fixed
Status: newclosed

I've applied the patch in hrev41581. To give you an idea of how much I felt still had to be cleaned up, I've attached a diff of your and "my" patch. Since I am looking forward to more patches from you, I hope those 2.5 hours were well spent :-). Some notes while its still fresh in my memory:

  • You don't need to call Close() on BEntries, BDirectories and BPaths and so on. These objects are supposed to be self-managed. When they go out of scope and are free'd, they will free the resources which they needed. You would only have to Close() when you keep the objects alive, but don't want them to keep file descriptors and such open.
  • When you create an object with "new", it is pointless to check the pointer against NULL, since plain new will never return NULL, but throw an std::bad_alloc exception instead. If you want a behavior like malloc(), you need to use new(std::nothrow) instead.
  • You need to reconsider what thread-safety means. If you want an object to be thread-safe, you should look closely at the changes I've made to DecorInfo.cpp with regards to the locking, it was not thread-safe at all before.
  • There were several instances of leaks in setdecor which I fixed, both in error code paths and in the regular path.
  • BString uses copy-on-write technique to be more efficient. When you do something like this: someString = someOtherString.String(), you are preventing this from working. When BString is given a const char pointer, it has no idea that this is actually the buffer of another BString object, so the ref-counting can't be employed. BString someString(someOtherString) will give you two BString objects pointing to the same internal memory chunk for efficiency, it's just increasing a ref-count.
  • The string operations to find a DecorInfo were much too involved, since you can just use BString::ICompare().
  • I noticed how you tried to find a DecorInfo both via its name and also its entry_ref name. You need to realize that the name of an entry_ref can never automatically change, for example because the user moves or renames a file. entry_refs work by keeping the inode number of the containing directory, the inode number of the device and the name of the entry in the directory. The user can only move the file in one way so that the entry_ref still points to it: By moving or renaming the parent directory. Specifically when renaming the entry itself, or moving it into another directory, the entry_ref becomes invalid. Since you tried to compare the name of the ref, the functionality was meaningless, since the fName and fRef members are updated at the same time. fName is actually useless because of that.

by stippi, 8 years ago

Attachment: cleanups.diff added

Diff of the patch before and after coding style cleanups and other improvements.

comment:23 by stippi, 8 years ago

I've attached a new version cleanups.diff again, since I had a bit of difficulties getting something clean, it's still not perfect but looking in Trac one can spot the coding style changes much more easily now. Hope you don't find it anal or anything, but rather productive for the future.

comment:24 by michaelvoliveira, 8 years ago

@looncraz, you could do something about upgrade user interface looking, I know that you know how ;)

in reply to:  22 comment:25 by looncraz, 8 years ago

Replying to stippi:

First, before I go into nitpicking at the risk of sounding like a thankless brat, I want to thank you for your hard work. You certainly made a lot of modifications!

I do see why you made most of the changes you did, and I'm grateful for you making it thread-safe ( I normally add it on after testing, just doing locking in obvious areas ). I'm, not sure how, but I completely forgot about BAutlock... how, I'll never know!! I must have megabytes of code using it!! LOL! Guess ~10 years of Java/PHP/HTML and c/++ on other platforms will do that!

And, I am astounded to see the patch applied! Thank you VERY much!!

What strikes me here is how much has changed from BeOS internally:

On BeOS ( well, Dano anyway ) BString copy-on-write didn't work and almost always led to issues ( garbled strings (, so I got into a habit of forcing copies. Should have expected Haiku to resolve that ( I think this was a Dano-only issue).

BeOS/Dano also would not Unset() some file system objects on destruction, causing obvious issues. Can't help but think that was on purpose, though...

BeOS did not throw exceptions through the API, and new would return NULL when out of memory. This is either a new behavior in Haiku or I got bad information back in the day. That said, I've never run into the problem and so only test at critical junctures. The rest of the time I assume other problems are going to be happening if we are out of memory, and doing all the checks just gets in the way of the code. That said, you added checks where I had none, and removed them where I did have them... I see no consistency in that regard, but I'm sure there is some unseen reason - please enlighten me.

  • You need to reconsider what thread-safety means. If you want an object to be thread-safe, you should look closely at the changes I've made to DecorInfo.cpp with regards to the locking, it was not thread-safe at all before.

DecorInfo/Utility was intentionally not thread-safe, even though there was some minimal locking in place for the future. I made notes in DecorInfo.h to that fact. One of my projects is capable of having 255*CPU_COUNT threads working on a single set of objects in memory, so I am very familiar with the ins & outs of thread safety, though I'll be making changes to BAutoLock in my more recent code where I seem to have forgotten all about it...

I didn't worry about leaks in setdecor, it is a single-run command-line app :-)

  • The string operations to find a DecorInfo were much too involved, since you can just use BString::ICompare().

I agree! Your way is MUCH nicer. Wasn't even aware of ICompare... (historically, I haven't used much BString, though).

  • I noticed how you tried to find a DecorInfo both via its name and also its entry_ref name.

Yes, that is an important feature you removed, thus changing behavior and removing features.

The likelihood of the name changing during operation is minimal and the ref.name matching is meant almost exclusively for the command line, lending to a shortcut-name so you have three case-insensitive ways to reference any decorator from the command-line. The reason is that the "fancy" names are verbose ( "Windows 95," "Stack and Tile," etc... ). I considered a more intelligent method (i.e. regex), but didn't think it was worth it for something that should be relegated to last-ditch-use (the command-line). That said, I am very much aware that entry_ref is a static structure and, thus, not live ;-)

Other than that, though, most of the issues seem to be with tabbing and white-space. I've long been white-space happy due to speed-reading practices ( also the reason I double-space after the end of sentences and why I prefer white space around parentheses ). That will be an extremely hard habit to break, but I will give it my best.

It also seems that Pe shows tabs differently than other IDEs, as some of my old code is not properly aligned. I suspect it is actually a font issue of some sort, but somewhere tabbing is getting messed up ( on my end ).

Thanks for the changes! I will be making more patches that affect this, but I assume that should be a new ticket and this one should be closed if it has been committed!

--The loon

in reply to:  24 comment:26 by looncraz, 8 years ago

Replying to michaelvoliveira:

@looncraz, you could do something about upgrade user interface looking, I know that you know how ;)

Already ahead of you.

What you don't see in the patch is all the references to PhOSDecorator I removed :-)

Looking for a new name, it is the Haiku decorator from PhOS.. HAHA!!

By far my favorite.

I have already begun the sliding tab animation portion, and soon I will have the "rounding" completed.

I got lost trying to find where to trick some alpha-blending for the decorators, somewhere it is being blocked, but I can't find exactly where. I won't be extending that to the windows, yet. But I suppose from there doing non-square windows with alpha-support wouldn't be much more than copying the principals. Performance will not be great unless I jump into some MMX/SSE which I think may be a no-no for platform specificness ( yeah, that IS a word.. LOL! ).

Stay tuned, if my changes are in alpha 3 then it'll make it much easier for me to release feature-reduced test decorators for input!

--The loon

Last edited 7 years ago by mmadia (previous) (diff)

comment:27 by stippi, 8 years ago

Hm, sorry for mixing up the reason for looking up via pretty name and ref name. I am not sure I broke it,since I left it in place in one spot. But I shall have another look.

in reply to:  27 comment:28 by looncraz, 8 years ago

Replying to stippi:

Hm, sorry for mixing up the reason for looking up via pretty name and ref name. I am not sure I broke it,since I left it in place in one spot. But I shall have another look.

Understandable, I should have used DecorInfo::ShortcutName() anyway!

That is why I did that in the first place, lol!!

If you haven't noticed, I have a memory issue, some bad RAM, I suppose.

DecorInfoUtility::FindDecorator(const BString&) is where the fix needs to be made, when you have a chance. ( I'd do it, but then I'd have to make a new ticket and all that good junk... ).

_FindDecor and FindDecorator have very different behavior. _FindDecor is absolute, finding only by path. FindDecorator should accept path, file name, shortcut name, & fancy name - all without case sensitivity.

Thanks again!

--The loon

comment:29 by stippi, 8 years ago

I've fixed it in hrev41602 (hopefully). Sorry again about that whole "entry_refs are not updated live" bogus, it was pretty stupid of me to think you could have made such a mistake. In my defense, the code wouldn't exactly enforce the right interpretation... :-)

Note: See TracTickets for help on using tickets.