| [23:10:18] | * farmBOT has joined #farmos |
| [10:36:11] | <mstenta[m]> | symbioquine: Hey! Just realized the farmOS map HCD call is tomorrow at 3 pm Eastern - not sure if you were included on the invite this time - can you join us? |
| [10:36:17] | <mstenta[m]> | paul121: it would be great to have you too! |
| [10:36:55] | <symbioquine[m]> | * Wasn't invited (yet) |
| [10:36:55] | <symbioquine[m]> | * Happy to join :) |
| [10:37:36] | <mstenta[m]> | Ah! OK I'll tell Sienna to add you |
| [10:37:38] | <mstenta[m]> | Great!! |
| [11:18:09] | <mstenta[m]> | paul121: What do you think about this? https://github.com/farmOS/farmOS/pull/575#discussion_r981380659 |
| [11:18:32] | <mstenta[m]> | I tested that with farmOS core and it seems like that would capture all the fields we want to affect. |
| [11:18:40] | <mstenta[m]> | Running it now on the PODS code to double check that one... |
| [11:25:18] | <mstenta[m]> | paul121: I know I said to get PRs together for 2.0.0-beta7 but go easy on me haha! |
| [11:25:34] | <mstenta[m]> | Dunno if we'll be able to merge everything this week. |
| [11:25:53] | <paul121[m]> | I don't have any more :-) |
| [11:26:04] | <symbioquine[m]> | I'll try to review the map related PRs later this morning (PST) |
| [11:26:26] | <mstenta[m]> | Do we need to squeeze those ones in for beta7 or could they wait? |
| [11:26:43] | <paul121[m]> | <mstenta[m]> "I tested that with farmOS core..." <- yeah, seems like that should work. I wonder if there is a way to get the field "column name" instead of the "field name" ? That might be the more generic solution |
| [11:27:23] | <symbioquine[m]> | mstenta[m]: It would be cool to get them in since they potentially enable building some totally new map-related workflows |
| [11:27:48] | <paul121[m]> | mstenta[m]: It would be nice but I can patch if we can't get them in |
| [11:28:09] | <paul121[m]> | do you guys have an easy way to test a map in a modal/off-canvas dialog? |
| [11:28:18] | <mstenta[m]> | Feels like a number of different things that might need more consideration time than I can give this week |
| [11:28:35] | <paul121[m]> | I was going to try and find something that doesn't involve custom code |
| [11:28:42] | <mstenta[m]> | (for a feature that I don't need yet haha) |
| [11:29:04] | <mstenta[m]> | But I agree it would be great! |
| [11:29:13] | <mstenta[m]> | If symbioquine can help review then I'm game to try! |
| [11:29:45] | <paul121[m]> | paul121[m]: tldr add this to an action link or task link definition that links to a page with a map:... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/25388ae8bc...) |
| [11:29:47] | <mstenta[m]> | I guess it's mainly the resize one that I feel needs the deepest thought |
| [11:30:44] | <mstenta[m]> | paul121: can the Gin CSS one wait? |
| [11:30:55] | <mstenta[m]> | Just trying to triage what I need to focus on in the limited time I have |
| [11:31:27] | <symbioquine[m]> | mstenta[m]: Yeah, I was wondering if 3 seconds is enough...? |
| [11:31:35] | <paul121[m]> | mstenta[m]: oh yeah not important. that will be a breaking change with Gin v4 |
| [11:33:21] | <paul121[m]> | (although they did seem to merge the breaking change into 3.x-dev which confuses me... I think the backwards compatibility layer still needs to be committed) |
| [11:38:58] | <paul121[m]> | <symbioquine[m]> "Yeah, I was wondering if 3..." <- I actually had it at 5 seconds and changed it to 3 before I pushed :D |
| [11:38:58] | <paul121[m]> | I kinda think 3 seconds is overkill. For me (on a relatively quick computer) it seems to only need the 0.5 seconds. I think we only need to account for the time between map creation & when the map is added to the dialog element. Not sure how that works though... I believe the internals of this are buried inside the jQuery Dialog UI library |
| [11:39:26] | <symbioquine[m]> | So no network latency involved? |
| [11:39:27] | <mstenta[m]> | timeouts give me the heebie jeebies |
| [11:39:44] | <mstenta[m]> | events are the proper solution |
| [11:39:46] | <mstenta[m]> | but i get the avoidance of jquery |
| [11:40:03] | <mstenta[m]> | but... maybe we include it only for this case? |
| [11:40:13] | <mstenta[m]> | (still haven't fully reviewed the code, just thinking out loud) |
| [11:40:41] | <mstenta[m]> | or somehow leave this up to the code implementing it? |
| [11:41:19] | <paul121[m]> | symbioquine[m]: I did a little test! (see PR). It seems that it doesn't matter though. It took much longer than 3 seconds to load everything, particularly it took more than 3 seconds to render the map *after* farmOS-map.js was loaded & it still worked. |
| [11:42:48] | <paul121[m]> | mstenta[m]: It's just hard to only identify this case :-/ |
| [11:43:15] | <mstenta[m]> | Right. |
| [11:43:39] | <mstenta[m]> | One thought might be to provide a `#detect_resize` property that adds the extra JS or something |
| [11:43:47] | <mstenta[m]> | (not saying that's good... just thinking through) |
| [11:44:13] | <paul121[m]> | One question: does calling `map.updateSize()` do anything "bad" if it doesn't need to be updated? I think that is what we want to avoid? |
| [11:44:18] | <mstenta[m]> | (would obviously be better if devs didn't need to worry about this... but i think the event is the "right" way to do that. timeouts are risky and generally bad practice as I understand it) |
| [11:44:55] | <mstenta[m]> | Probably not? Just extraneous processing I suppose. |
| [11:45:16] | <paul121[m]> | mstenta[m]: The issue is that any page could be loaded into a dialog. For example iirc, how Pedro loads the log edit form into off-canvas |
| [11:45:57] | <paul121[m]> | mstenta[m]: But! We can first check if the map "has a size". If it does, no need to update. |
| [11:46:50] | <paul121[m]> | So then we are only "updating the size" for maps intended to be hidden, which yeah is extra processing |
| [11:47:42] | <paul121[m]> | (might need a slight change in my PR to ensure we only update if needed) |
| [11:47:42] | <mstenta[m]> | It seems like the core issue here is that the current event system uses jQuery, right? I wonder if there's a plan to move away from jQuery? |
| [11:48:29] | <paul121[m]> | mstenta[m]: yeah let me see if I can find the issue... but it is a ways out |
| [11:49:43] | <paul121[m]> | https://www.drupal.org/project/drupal/issues/3176441 |
| [11:51:29] | <mstenta[m]> | I don't love the idea of adding jQuery, but it feels like that might be the "right" approach to this. With the expectation that the jQuery dependency will be removed upstream eventually. |
| [11:52:12] | <mstenta[m]> | Maybe here's another question: is jQuery already being included for other things in most of the pages that use maps? |
| [11:52:29] | <mstenta[m]> | Maybe adding jQuery won't actually change anything right now. |
| [11:53:44] | <paul121[m]> | Yeah it is. Gin still uses it, but will be removing it soon. There are likely other things using jQuery too, though |
| [11:54:12] | <paul121[m]> | But... I actually tried using the jQuery event and seemed to have the same issue. The map isn't included in the dialog yet |
| [11:55:56] | <paul121[m]> | Anyways, yeah this would be a nice to have for 2.7, certainly other things are a priority |
| [11:56:14] | <paul121[m]> | (eg: maps don't even work in the dialog without fixing chunk loading) |
| [11:56:44] | <mstenta[m]> | ? |
| [11:56:45] | <mstenta[m]> | Ok yea I was gonna say maybe you can find a way to do this in a custom module until we can give this more thought |
| [11:57:04] | <paul121[m]> | I think I'll just patch instead haha there isn't really another easy way |
| [12:10:19] | <evered[m]> | If anyone needs database (SQL) eyes, I am happy to give a look.. I have a master's in computer science with emphasis database systems. |
| [12:10:44] | <evered[m]> | unless farmOS uses ORM... |
| [12:13:29] | <symbioquine[m]> | evered[m]: Maybe you have inputs regarding how bad in practice the extra joins will be in [this PR](https://github.com/farmOS/farmOS/pull/571) from a performance/potential-DOS perspective... |
| [12:16:05] | <evered[m]> | multiple joins can be an extreme bottle-neck... please stand by folding laundry |
| [12:22:12] | * polo__ has joined #farmos |
| [12:57:53] | * polo__ has quit (Read error: Connection reset by peer) |
| [13:10:06] | * polo__ has joined #farmos |
| [13:13:54] | <mstenta[m]> | evered: Agreed - what this does is balances that against the need to remove duplicate rows when a filter is applied. |
| [13:14:02] | <mstenta[m]> | paul121: What do you think of this? https://github.com/farmOS/farmOS/pull/575/files |
| [13:14:10] | <mstenta[m]> | Works in all the testing I've tried. |
| [13:20:59] | * polo__ has quit (Read error: Connection reset by peer) |
| [13:38:58] | * polo__ has joined #farmos |
| [13:47:42] | <mstenta[m]> | paul121: any qualms with me merging https://github.com/farmOS/farmOS/pull/537? |
| [13:47:49] | <mstenta[m]> | Or should we wait? |
| [13:54:56] | <paul121[m]> | I haven't tested the code myself but if it works then it seems like it should be ready! |
| [14:19:11] | <symbioquine[m]> | <symbioquine[m]> "I'll try to review the map..." <- Might be running a bit late on this... Internet is down here 🥺 |
| [14:25:32] | * polo__ has quit (Quit: My MacBook has gone to sleep. ZZZzzz…) |
| [14:30:45] | <mstenta[m]> | No worries symbioquine |
| [14:41:51] | * polo__ has joined #farmos |
| [14:51:15] | <mstenta[m]> | I have the following PRs lined up to merge:... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/ed7122c7a5...) |
| [14:55:23] | <mstenta[m]> | paul121: Should I take the time to review https://github.com/farmOS/farmOS/pull/573 or is it ok to do that in the next release? Are you waiting on it for anything? (Haven't looked closely... just triaging right now) |
| [14:59:55] | <paul121[m]> | That can wait until next release. It's only code improvements & a super minor bug fix, I'm not dependent on it. |
| [15:00:48] | <mstenta[m]> | Cool! Thanks :-) |
| [15:02:28] | <mstenta[m]> | this is going to be a pretty big release! 20+ changelog entries! :-) |
| [15:51:55] | * polo__ has quit (Quit: My MacBook has gone to sleep. ZZZzzz…) |
| [16:37:15] | * Guest5055 has joined #farmos |
| [16:39:03] | <mstenta[m]> | paul121: I'm just giving this some thought... https://www.drupal.org/project/farm/issues/3311264 |
| [16:39:23] | <mstenta[m]> | Just realized: we define the `client_id` fields as a *bundle* field, not a base field |
| [16:39:56] | <mstenta[m]> | > I was able to add this as a _Bundle field_ (instead of a Base field) using `hook_entity_base_field_info()`. This puts the new field in its own DB table, just as the field storage definitions do for the consumer fields mentioned above. |
| [16:40:00] | <mstenta[m]> | https://www.drupal.org/node/3207716 |
| [16:41:02] | <mstenta[m]> | I wonder if that will actually make the transition any easier... considering that Consumers will be adding it as a base field, which will add it to the `consumers_field_data` table, rather than to a separate table like we are doing |
| [16:41:34] | <mstenta[m]> | Need to think through how all the update hooks will interact... 🤔 |
| [16:43:28] | <paul121[m]> | what do you mean, we actually thought ahead about this? 1.5 years ago and forgot?? 😁 |
| [16:43:40] | <paul121[m]> | I did not notice it was in a bundle field |
| [16:44:14] | <mstenta[m]> | hahaha |
| [16:44:20] | <mstenta[m]> | well it might be a happy accident |
| [16:44:39] | <paul121[m]> | well a base and bundle field with the same name is no bueno |
| [16:44:53] | <mstenta[m]> | yes. we're not out of the fire yet. |
| [16:45:00] | <mstenta[m]> | https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension... |
| [16:45:01] | <mstenta[m]> | but we might be able to swing it... |
| [16:45:08] | <paul121[m]> | can we run hooks in a specific order? alter the order? |
| [16:45:20] | <mstenta[m]> | `hook_update_depenencies()` lets you define which `hook_update_N()` needs to run first |
| [16:45:34] | <mstenta[m]> | so we can add ours before the Consumers |
| [16:45:47] | <mstenta[m]> | thinking through what that will need to do now... |
| [16:46:05] | <paul121[m]> | oh wow. that seems perfect |
| [16:46:09] | <mstenta[m]> | save the existing `client_id` values somewhere (maybe just a temporary state variable?)... |
| [16:46:16] | <mstenta[m]> | uninstall the existing field... |
| [16:46:32] | <mstenta[m]> | then let the consumer update hook(s) run (there are two of them) |
| [16:46:42] | <mstenta[m]> | then run another update hook of our own to copy the `client_id` values back |
| [16:46:44] | <mstenta[m]> | maybe? |
| [16:47:17] | <paul121[m]> | Seems logical |
| [16:47:17] | <mstenta[m]> | > maybe just a temporary state variable? |
| [16:47:18] | <mstenta[m]> | i don't think there are ever more than a handful, if that... so this should work I think? |
| [16:47:26] | <paul121[m]> | yeah |
| [16:47:55] | <mstenta[m]> | might sketch up a wip commit quick to think it through... |
| [16:47:56] | <paul121[m]> | or, we could rename our field so it doesn't conflict as step 1, and then after consumers is updated copy the values over & finally remove our field |
| [16:48:12] | <paul121[m]> | that seems a bit more challenging tho |
| [16:48:14] | <mstenta[m]> | yea thought about that too... it's more work |
| [16:48:15] | <mstenta[m]> | yea |
| [16:48:32] | <mstenta[m]> | renaming a field basically means creating a new one, migrating to it, deleting the old one |
| [16:48:49] | <mstenta[m]> | might not be necessary |
| [17:00:19] | * Guest5055 has quit (Quit: My MacBook has gone to sleep. ZZZzzz…) |
| [17:00:30] | <mstenta[m]> | yea i think this should work... in theory |
| [17:02:39] | <mstenta[m]> | draft commit: https://github.com/mstenta/farmOS/commit/bd4d8bb91ab64b453b1872e704f12de... |
| [17:02:57] | <mstenta[m]> | (still needs the actual logic, but scaffolds the hooks and comments...) |
| [17:17:06] | <paul121[m]> | makes sense! |
| [17:17:31] | <mstenta[m]> | sketching up the actual logic right now... pretty simple (if it works - we'll have to see) |
| [17:20:25] | <paul121[m]> | making another PR... simple but not urgent ;-) |
| [17:22:15] | <mstenta[m]> | paul121: commit w/ logic -> https://github.com/mstenta/farmOS/commit/65d5820165686c96ed1fdd9d97b2182... |
| [17:22:20] | <mstenta[m]> | (100% untested) |
| [17:23:14] | <paul121[m]> | so with the update hook dependencies... say we merged these changes in now, before the consume module has a release with the update hook |
| [17:23:48] | <paul121[m]> | with the farm_api update hooks not be attempted until a relevant consumers release is included? |
| [17:23:53] | <mstenta[m]> | oh hmmmmm |
| [17:23:54] | <paul121[m]> | s/with/will/, s/farm_api/farm\_api/ |
| [17:23:55] | <mstenta[m]> | that's a really good question |
| [17:24:21] | <mstenta[m]> | let me see what happens when i run `drush updb` locally! |
| [17:28:06] | * polo___ has joined #farmos |
| [17:28:26] | <mstenta[m]> | hmm, I don't think it worked as expected... it just ran the updates |
| [17:30:02] | <mstenta[m]> | (and revealed a bug in my code) 😅 |
| [17:41:01] | <mstenta[m]> | Hmm need to figure out the right way to uninstall the field definition |
| [17:41:09] | <mstenta[m]> | This is NOT it:... (full message at <https://libera.ems.host/_matrix/media/r0/download/libera.chat/d8a3de7693...) |
| [17:41:43] | <mstenta[m]> | `FieldStorageConfig::loadByName('consumer', 'client_id')` is `null` |
| [17:41:51] | <mstenta[m]> | because it's not a config field :-) |
| [17:42:48] | <paul121[m]> | :-/ |
| [17:44:17] | <mstenta[m]> | looks like `entity` module might have the logic we need... |
| [17:44:45] | <mstenta[m]> | in `BundlePluginInstaller::uninstallBundles()` |
| [17:45:42] | <mstenta[m]> | to be continued... dinner time :-) |
| [18:12:41] | * polo___ has quit (Changing host) |
| [18:12:41] | * polo___ has joined #farmos |
| [18:12:42] | * polo___ is now known as GiverOfDomains |
| [18:12:48] | * GiverOfDomains is now known as polo |