It is currently December 12th, 2019, 8:24 pm

[!EditSkin config] opening the editor for the currently open variant of the skin if a file isn't provided

Report bugs with the Rainmeter application and suggest features.
User avatar
jsmorley
Developer
Posts: 19864
Joined: April 19th, 2009, 11:02 pm
Location: Fort Hunt, Virginia, USA

Re: [!EditSkin config] opening the editor for the currently open variant of the skin if a file isn't provided

jsmorley » November 18th, 2019, 7:19 pm

Yamajac wrote:
November 18th, 2019, 7:15 pm
O, yea, it'd basically just replace the "There are extra args but they aren't valid" error message. So if there's one arg, and it's a valid skin, it passes the bang to that skin in the same way other bangs would. Then another case gets added to Skin::DoBang() and bippity boppity edit skin works ezpz.

I'd have to actually have it set up in a proper IDE to be 100% sure that it all works as intended with no side effects but from a cursory glance it seems like that'd be all that's required and there shouldn't be any conflicts. So I'll probably send a pull request later today, unless my cursory glance failed me. Which I expect.
Not sure it changes that thinking, but it should be kept in mind that this is a slightly different use of the "config" parameter in a bang. We need to be careful that it treats config AND file differently than it treats JUST config. In all other bangs, "config" has one meaning. It is to pass the bang off to another running skin. In this case, "config" can have two somewhat distinct meanings.
User avatar
Yamajac
Posts: 132
Joined: June 30th, 2014, 8:44 am

Re: [!EditSkin config] opening the editor for the currently open variant of the skin if a file isn't provided

Yamajac » November 18th, 2019, 7:47 pm

jsmorley wrote:
November 18th, 2019, 7:19 pm
Not sure it changes that thinking, but it should be kept in mind that this is a slightly different use of the "config" parameter in a bang. We need to be careful that it treats config AND file differently than it treats JUST config. In all other bangs, "config" has one meaning. It is to pass the bang off to another running skin. In this case, "config" can have two distinct meanings.
Well, DoEditSkinBang() just calls GetRainmeter(config, file).EditSkinFile() with either the config+file provided or the current config + current file depending on whether 2 or 0 args were provided to the bang. I'll just add in a third check for 1 arg to pass it to the provided config, add a Skin::EditFile() or something so we aren't f---ing up DRY, and it'll all work identically except now you can have just a config with no file.


So the bang will go from !EditSkin OR !EditSkin Config File to !EditSkin, !EditSkin ACTIVECONFIG OR !EditSKin Config File


Anyway, IDK what this discussions really about anymore so I'll just send a pull request later and you guys tell me if I did good or not.
User avatar
Yamajac
Posts: 132
Joined: June 30th, 2014, 8:44 am

Re: [!EditSkin config] opening the editor for the currently open variant of the skin if a file isn't provided

Yamajac » November 18th, 2019, 9:21 pm

Well that was fun.
User avatar
jsmorley
Developer
Posts: 19864
Joined: April 19th, 2009, 11:02 pm
Location: Fort Hunt, Virginia, USA

Re: [!EditSkin config] opening the editor for the currently open variant of the skin if a file isn't provided

jsmorley » November 18th, 2019, 10:19 pm

Yamajac wrote:
November 18th, 2019, 9:21 pm
Well that was fun.
Done. Initial testing seems fine!

https://github.com/rainmeter/rainmeter/commits/master
Merge pull request #209 from Yamajac/master

Change !EditSkin so it can be passed a "config" without a .ini "file", and it will edit the currently running skin in the defined config.

So if:

[!EditSkin] : Edits the .ini file for the currently running skin the bang is in
[!EditSkin "ConfigName"] : Edits the .ini file for the currently running skin in the defined config
[!EdiitSkin "ConfigName" "FileName.ini"] : Edits the explicitly defined .ini file, running or not
One thing to note about !EditSkin (always been this way) is that only "skin" .ini files can be edited this way, (not .inc for instance), and the .ini file must have been "scanned" by Rainmeter during Rainmeter startup or Refresh All. The skin file must exist in the "known pool" of Skins in Rainmeter.
User avatar
jsmorley
Developer
Posts: 19864
Joined: April 19th, 2009, 11:02 pm
Location: Fort Hunt, Virginia, USA

Re: [!EditSkin config] opening the editor for the currently open variant of the skin if a file isn't provided

jsmorley » November 18th, 2019, 10:29 pm

User avatar
Yamajac
Posts: 132
Joined: June 30th, 2014, 8:44 am

Re: [!EditSkin config] opening the editor for the currently open variant of the skin if a file isn't provided

Yamajac » November 18th, 2019, 10:37 pm

jsmorley wrote:
November 18th, 2019, 10:19 pm
Done. Initial testing seems fine!

https://github.com/rainmeter/rainmeter/commits/master



One thing to note about !EditSkin (always been this way) is that only "skin" .ini files can be edited this way, (not .inc for instance), and the .ini file must have been "scanned" by Rainmeter during Rainmeter startup or Refresh All. The skin file must exist in the "known pool" of Skins in Rainmeter.
Awesome! I'm so happy about this lol, never made a pull request before so that was fun to do.
User avatar
jsmorley
Developer
Posts: 19864
Joined: April 19th, 2009, 11:02 pm
Location: Fort Hunt, Virginia, USA

Re: [!EditSkin config] opening the editor for the currently open variant of the skin if a file isn't provided

jsmorley » November 18th, 2019, 10:39 pm

Yamajac wrote:
November 18th, 2019, 10:37 pm
Awesome! I'm so happy about this lol, never made a pull request before so that was fun to do.
Well good job! I'll get a new beta out shortly!
User avatar
Brian
Developer
Posts: 1932
Joined: November 24th, 2011, 1:42 am
Location: Utah

Re: [!EditSkin config] opening the editor for the currently open variant of the skin if a file isn't provided

Brian » November 18th, 2019, 11:40 pm

Yamajac wrote:
November 18th, 2019, 10:37 pm
Awesome! I'm so happy about this lol, never made a pull request before so that was fun to do.
Good job on your first PR.

A couple of notes:
1. This doesn't work for config names that are 1 character in length.
2. Simply sending the config argument to "GetSkin" will return a valid skin or nullptr...all those extra validity checks are redundant.
3. It felt like an extra function in Skin.cpp just to call EditSkinFile was not necessary, so I removed it.
4. There was a extra "!" in the error message. But that is pretty minor.

I went ahead and made some changes, here is my commit: https://github.com/rainmeter/rainmeter/commit/5b989f4089c2f2b7ca4eb88c8305c7c0080e01bb

All in all, this was a nice addition giving skin authors more flexibility in making skins. Thanks.

-Brian
User avatar
Yamajac
Posts: 132
Joined: June 30th, 2014, 8:44 am

Re: [!EditSkin config] opening the editor for the currently open variant of the skin if a file isn't provided

Yamajac » November 18th, 2019, 11:58 pm

Brian wrote:
November 18th, 2019, 11:40 pm
Good job on your first PR.

A couple of notes:
1. This doesn't work for config names that are 1 character in length.
2. Simply sending the config argument to "GetSkin" will return a valid skin or nullptr...all those extra validity checks are redundant.
3. It felt like an extra function in Skin.cpp just to call EditSkinFile was not necessary, so I removed it.
4. There was a extra "!" in the error message. But that is pretty minor.

I went ahead and made some changes, here is my commit: https://github.com/rainmeter/rainmeter/commit/5b989f4089c2f2b7ca4eb88c8305c7c0080e01bb

All in all, this was a nice addition giving skin authors more flexibility in making skins. Thanks.

-Brian

Cool! Thanks for the feedback! I'll keep that in mind if I ever end up working on anything else.


I do have a question though, at what point would the extra function in Skin.cpp be worth it to you? In my opinion if I'm ever calling the same function with the same parameters more than once I want to add a layer of abstraction to make any changes/additions easier in the future.

The rest of the stuff I probably could've figured out if I just put more time into it, but I'd like to hear your views on the extra function at least if you have the time.
User avatar
Brian
Developer
Posts: 1932
Joined: November 24th, 2011, 1:42 am
Location: Utah

Re: [!EditSkin config] opening the editor for the currently open variant of the skin if a file isn't provided

Brian » November 19th, 2019, 12:33 am

Yamajac wrote:
November 18th, 2019, 11:58 pm
I do have a question though, at what point would the extra function in Skin.cpp be worth it to you? In my opinion if I'm ever calling the same function with the same parameters more than once I want to add a layer of abstraction to make any changes/additions easier in the future.
For me, it comes down to a cost/benefit issue. The only real "easier" part about this type of "pass through function" is the actual physical typing of it. Sure it may be called more than once, but in all likelihood, it won't be called in many places.

There is also a very small timing issue where if the bang is in the middle of executing, and the other skin is unloaded, it could cause a problem. It's better to just let the main Rainmeter thread handle it.

As the size of our project grows, it's important to keep the compile time down, and limit any extra memory wasted on function stacks. In this particular case, it wouldn't be an issue...but if we did this in lots of other areas with the size of our codebase, it could slow things down quite a bit.

-Brian