Change `CharacterCache::{character, width}` methods to take `&self` rather than `&mut self`.

[mitchmindtree]

This is just an idea for improving the usability of implementations of CharacterCache. From a related conrod issue:

Change the CharacterCache .character and .width methods to take &self instead of &mut self, requiring implementations to use a lock or RefCell<> over the underlying HashMap itself.

CharacterCache is often required for calculating the length of strings or rendering text. The only reason it is mutable is so that it can update the cache with characters in the occasion that it doesn't yet have them. As seen in conrod, this mutability requirement is prone to causing subtle ownership issues due to how frequently the methods are required.

All implementations of CharacterCache so far use a HashMap as the underlying cache - I wonder if these implementations could instead use a RefCell<HashMap> so that we can take &self instead of &mut self? Or perhaps a Mutex<HashMap> would be safer, so that rather than panic!ing when the HashMap is already borrowed, we can just wait for the lock (on the rare occasion this happens it should never take long as the cache is only ever updated with a single character at a time).

[bvssvni]

Sound good. I guess the idea with Mutex<HashMap> to use it with multiple threads?

[mitchmindtree]

Yeah, do you think it's necessary? Maybe RefCell is fine?

[bvssvni]

We could try Mutex<HashMap> and see how it goes.

[mitchmindtree]

Just had a go at this and ran into some borrowing issues - as the return type of CharacterCache::character is &Character, this rules out both internal Mutex and RefCell as both rely on the lifetime of the reference being no greater than the scope in which they are locked/borrowed. Perhaps we'll just have to wrap the whole CharacterCache in a RefCell when retaining immutable signatures is desired.

[bvssvni]

Yeah, that sounds like a reasonable solution. Maybe a good idea to make mental note that designing a data structure internally for sharing does not always work out, and one should instead put the whole thing in a suitable smart pointer.

Closing?

[mitchmindtree]

Yep, mental note taken :+1:

[milibopp]

Couldn't CharacterCache::character return some associated type with the bound Deref<Target=Character> to be more generic? Or would this also not work out lifetime-wise?

[bvssvni]

@aepsil0n I am not sure what you mean, but I think the lifetime problem is orthogonal as you still need a mutable reference to the CharacterCache.

[milibopp]

Well, as I understand the problem, the current CharacterCache trait has a method like this:

fn character(&mut self, font_size: FontSize, ch: char) -> &Character<Self::Texture>;

My suggestion was to change this to use an associated type like this:

type CharRef: Deref<Target=Character<Self::Texture>>;
fn character(&self, ...) -> CharRef;

An implementation could then use some lock guard instead of a reference.

But now that I've written this down I see the problem: we need to tie the lifetime of CharRef to the lifetime of &self. Unfortunately this requires either higher-kinded types or a workaround similar to the IntoIterator trait from the standard library. Not sure whether that is applicable here. Anyway, using the workaround the trait would then become (and the other method would have to change similarly):

type CharRef: Deref<Target=Character<Self::Texture>>;
fn character(self, ...) -> CharRef;

Note that self is taken by value here. Implementations would be generic over the lifetime 'a in &'a ConcreteCache instead of directly for ConcreteCache. Not sure whether that translates so well to this use case.

On first sight it looked like there was an easy solution but I suppose it's not quite that simple. In any case I feel like this internal mutability is something worth hiding as the mutability here does not compromise referential transparency. But well, it wouldn't be a huge problem if that responsibility were pushed to client-side code.