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.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.
It is currently March 29th, 2024, 2:52 pm
[!EditSkin config] opening the editor for the currently open variant of the skin if a file isn't provided
-
- Developer
- Posts: 22628
- 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
-
- Posts: 134
- 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
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.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.
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.
-
- Posts: 134
- Joined: June 30th, 2014, 8:44 am
-
- Developer
- Posts: 22628
- 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
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.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
-
- Developer
- Posts: 22628
- Joined: April 19th, 2009, 11:02 pm
- Location: Fort Hunt, Virginia, USA
-
- Posts: 134
- 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
Awesome! I'm so happy about this lol, never made a pull request before so that was fun to do.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.
-
- Developer
- Posts: 22628
- 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
Well good job! I'll get a new beta out shortly!
-
- Developer
- Posts: 2674
- 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
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
-
- Posts: 134
- 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
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.
-
- Developer
- Posts: 2674
- 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
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.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.
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