Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove event support #880

Merged
merged 6 commits into from
Oct 12, 2023
Merged

Remove event support #880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c8daefb
Remove event support
osa1 Oct 10, 2023
f5f8983
Remove more stuff
osa1 Oct 10, 2023
2d303f3
Simplification
osa1 Oct 10, 2023
f452b9f
Update return types of ensure methods
osa1 Oct 10, 2023
26713c3
Merge remote-tracking branch 'origin/master' into remove_event_stuff
osa1 Oct 10, 2023
484b305
Sync changes from the CL
osa1 Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions protobuf/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,19 @@
## 4.0.0-dev

* The following types and members are now removed:

- `PbEventMixin`
- `PbFieldChange`
- `EventBuffer`
- `GeneratedMessage.createRepeatedField`
- `GeneratedMessage.createMapField`

These were used to implement events, which are unused internally. To keep API
surface small (to make it easier to change the library or migrate to another
library) these types and members are removed. ([#738])

[#738]: https://github.com/google/protobuf.dart/issues/738

## 3.1.0

* `CodedBufferReader` `readBytes` now copies the returned bytes to avoid
Expand Down
3 changes: 0 additions & 3 deletions protobuf/lib/meta.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@ const GeneratedMessage_reservedNames = <String>[
'clone',
'copyWith',
'createEmptyInstance',
'createMapField',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should leave these? Otherwise the names will no longer be disambiguated breaking existing code?
Not sure what is better here...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you suggest we keep them indefinitely, or remove later?

If you want to remove later, this is already a breaking change so we could do it now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - it is either remove

  • now or
  • never.

Not sure what is better. We could check if we have any usages inside google3. These names are probably quite rare - so maybe it is better to remove now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the CL globally internally it doesn't break anything.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, in that case I vote for just removing them

'createRepeatedField',
'eventPlugin',
'extensionsAreInitialized',
'freeze',
'getDefaultForField',
Expand Down
1 change: 0 additions & 1 deletion protobuf/lib/protobuf.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ part 'src/protobuf/coded_buffer.dart';
part 'src/protobuf/coded_buffer_reader.dart';
part 'src/protobuf/coded_buffer_writer.dart';
part 'src/protobuf/consts.dart';
part 'src/protobuf/event_plugin.dart';
part 'src/protobuf/exceptions.dart';
part 'src/protobuf/extension.dart';
part 'src/protobuf/extension_field_set.dart';
Expand Down
31 changes: 0 additions & 31 deletions protobuf/lib/src/protobuf/event_plugin.dart
View file
Open in desktop

This file was deleted.

18 changes: 5 additions & 13 deletions protobuf/lib/src/protobuf/extension_field_set.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class _ExtensionFieldSet {
///
/// If it doesn't exist, creates the list and saves the extension.
/// Suitable for public API and decoders.
List<T> _ensureRepeatedField<T>(Extension<T> fi) {
PbList<T> _ensureRepeatedField<T>(Extension<T> fi) {
assert(!_isReadOnly);
assert(fi.isRepeated);
assert(fi.extendee == '' || fi.extendee == _parent._messageName);
Expand All @@ -50,17 +50,17 @@ class _ExtensionFieldSet {
return _addInfoAndCreateList(fi);
}

List<T> _getList<T>(Extension<T> fi) {
PbList<T> _getList<T>(Extension<T> fi) {
final value = _values[fi.tagNumber];
if (value != null) return value;
_checkNotInUnknown(fi);
if (_isReadOnly) return List<T>.unmodifiable(const []);
if (_isReadOnly) return PbList<T>.unmodifiable();
return _addInfoAndCreateList<T>(fi);
}

List<T> _addInfoAndCreateList<T>(Extension<T> fi) {
PbList<T> _addInfoAndCreateList<T>(Extension<T> fi) {
_validateInfo(fi);
final newList = fi._createRepeatedField(_parent._message!);
final newList = fi._createRepeatedField();
_addInfoUnchecked(fi);
_setFieldUnchecked(fi, newList);
return newList;
Expand All @@ -76,10 +76,6 @@ class _ExtensionFieldSet {
void _clearField(Extension fi) {
_ensureWritable();
_validateInfo(fi);
final eventPlugin = _parent._eventPlugin;
if (eventPlugin != null && eventPlugin.hasObservers) {
eventPlugin.beforeClearField(fi);
}
_values.remove(fi.tagNumber);
}

Expand Down Expand Up @@ -134,10 +130,6 @@ class _ExtensionFieldSet {
}

void _setFieldUnchecked(Extension fi, value) {
final eventPlugin = _parent._eventPlugin;
if (eventPlugin != null && eventPlugin.hasObservers) {
eventPlugin.beforeSetField(fi, value);
}
// If there was already an unknown field with the same tag number,
// overwrite it.
_parent._unknownFields?.clearField(fi.tagNumber);
Expand Down
23 changes: 10 additions & 13 deletions protobuf/lib/src/protobuf/field_info.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,23 +215,20 @@ class FieldInfo<T> {
}
}

/// Creates a repeated field to be attached to the given message.
///
/// Delegates actual list creation to the message, so that it can
/// be overridden by a mixin.
List<T> _createRepeatedField(GeneratedMessage m) {
/// Creates a repeated field.
PbList<T> _createRepeatedField() {
assert(isRepeated);
return m.createRepeatedField<T>(tagNumber, this);
return PbList<T>(check: check!);
}

/// Same as above, but allow a tighter typed List to be created.
List<S> _createRepeatedFieldWithType<S extends T>(GeneratedMessage m) {
/// Same as above, but allow a tighter typed [PbList] to be created.
PbList<S> _createRepeatedFieldWithType<S extends T>() {
assert(isRepeated);
return m.createRepeatedField<S>(tagNumber, this as FieldInfo<S>);
return PbList<S>(check: check!);
}

/// Convenience method to thread this FieldInfo's reified type parameter to
/// _FieldSet._ensureRepeatedField.
/// `_FieldSet._ensureRepeatedField`.
List<T> _ensureRepeatedField(BuilderInfo meta, _FieldSet fs) {
return fs._ensureRepeatedField<T>(meta, this);
}
Expand Down Expand Up @@ -292,12 +289,12 @@ class MapFieldInfo<K, V> extends FieldInfo<PbMap<K, V>?> {
FieldInfo get valueFieldInfo =>
mapEntryBuilderInfo.fieldInfo[PbMap._valueFieldNumber]!;

Map<K, V> _ensureMapField(BuilderInfo meta, _FieldSet fs) {
PbMap<K, V> _ensureMapField(BuilderInfo meta, _FieldSet fs) {
return fs._ensureMapField<K, V>(meta, this);
}

Map<K, V> _createMapField(GeneratedMessage m) {
PbMap<K, V> _createMapField() {
assert(isMapField);
return m.createMapField<K, V>(tagNumber, this);
return PbMap<K, V>(keyFieldType, valueFieldType);
}
}
56 changes: 10 additions & 46 deletions protobuf/lib/src/protobuf/field_set.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ void _throwFrozenMessageModificationError(String messageName,
/// JavaScript.
class _FieldSet {
final GeneratedMessage? _message;
final EventPlugin? _eventPlugin;

/// The value of each non-extension field in a fixed-length array.
/// The index of a field can be found in [FieldInfo.index].
Expand Down Expand Up @@ -74,7 +73,7 @@ class _FieldSet {
/// the index is not present, the oneof field is unset.
final Map<int, int>? _oneofCases;

_FieldSet(this._message, BuilderInfo meta, this._eventPlugin)
_FieldSet(this._message, BuilderInfo meta)
: _values = _makeValueList(meta.byIndex.length),
_oneofCases = meta.oneofs.isEmpty ? null : <int, int>{};

Expand Down Expand Up @@ -227,10 +226,6 @@ class _FieldSet {
assert(tagNumber == fi.tagNumber);

// Clear a non-extension field
final eventPlugin = _eventPlugin;
if (eventPlugin != null && eventPlugin.hasObservers) {
eventPlugin.beforeClearField(fi);
}
_values[fi.index!] = null;

final oneofIndex = meta.oneofs[tagNumber];
Expand Down Expand Up @@ -302,7 +297,7 @@ class _FieldSet {
/// Creates and stores the repeated field if it doesn't exist.
/// If it's an extension and the list doesn't exist, validates and stores it.
/// Suitable for decoders.
List<T> _ensureRepeatedField<T>(BuilderInfo meta, FieldInfo<T> fi) {
PbList<T> _ensureRepeatedField<T>(BuilderInfo meta, FieldInfo<T> fi) {
assert(!_isReadOnly);
assert(fi.isRepeated);
if (fi.index == null) {
Expand All @@ -311,7 +306,7 @@ class _FieldSet {
final value = _getFieldOrNull(fi);
if (value != null) return value;

final newValue = fi._createRepeatedField(_message!);
final newValue = fi._createRepeatedField();
_setNonExtensionFieldUnchecked(meta, fi, newValue);
return newValue;
}
Expand All @@ -324,9 +319,9 @@ class _FieldSet {
final value = _getFieldOrNull(fi);
if (value != null) return value;

final newValue = fi._createMapField(_message!);
final newValue = fi._createMapField();
_setNonExtensionFieldUnchecked(meta, fi, newValue);
return newValue as PbMap<K, V>;
return newValue;
}

/// Sets a non-extended field and fires events.
Expand All @@ -341,14 +336,6 @@ class _FieldSet {
_oneofCases![oneofIndex] = tag;
}

// It is important that the callback to the observers is not moved to the
// beginning of this method but happens just before the value is set.
// Otherwise the observers will be notified about 'clearField' and
// 'setField' events in an incorrect order.
final eventPlugin = _eventPlugin;
if (eventPlugin != null && eventPlugin.hasObservers) {
eventPlugin.beforeSetField(fi, value);
}
_values[fi.index!] = value;
}

Expand Down Expand Up @@ -394,7 +381,7 @@ class _FieldSet {
return fi.readonlyDefault;
}

final list = fi._createRepeatedFieldWithType<T>(_message!);
final list = fi._createRepeatedFieldWithType<T>();
_setNonExtensionFieldUnchecked(_meta, fi, list);
return list;
}
Expand All @@ -412,7 +399,7 @@ class _FieldSet {
PbMap<K, V>(fi.keyFieldType, fi.valueFieldType));
}

final map = fi._createMapField(_message!);
final map = fi._createMapField();
_setNonExtensionFieldUnchecked(_meta, fi, map);
return map;
}
Expand Down Expand Up @@ -486,10 +473,6 @@ class _FieldSet {
if (value == null) {
_$check(index, value); // throw exception for null value
}
final eventPlugin = _eventPlugin;
if (eventPlugin != null && eventPlugin.hasObservers) {
eventPlugin.beforeSetField(_nonExtensionInfoByIndex(index), value);
}
final meta = _meta;
final tag = meta.byIndex[index].tagNumber;
final oneofIndex = meta.oneofs[tag];
Expand All @@ -516,25 +499,8 @@ class _FieldSet {
if (_unknownFields != null) {
_unknownFields!.clear();
}

final extensions = _extensions;

final eventPlugin = _eventPlugin;
if (eventPlugin != null && eventPlugin.hasObservers) {
for (final fi in _infos) {
if (_values[fi.index!] != null) {
eventPlugin.beforeClearField(fi);
}
}
if (extensions != null) {
for (final key in extensions._tagNumbers) {
final fi = extensions._getInfoOrNull(key)!;
eventPlugin.beforeClearField(fi);
}
}
}
if (_values.isNotEmpty) _values.fillRange(0, _values.length, null);
extensions?._clearValues();
_extensions?._clearValues();
}

bool _equals(_FieldSet o) {
Expand Down Expand Up @@ -882,15 +848,13 @@ class _FieldSet {
if (fieldInfo.isMapField) {
final PbMap? map = _values[index];
if (map != null) {
_values[index] = (fieldInfo as MapFieldInfo)
._createMapField(_message!)
_values[index] = (fieldInfo as MapFieldInfo)._createMapField()
..addAll(map);
}
} else if (fieldInfo.isRepeated) {
final PbList? list = _values[index];
if (list != null) {
_values[index] = fieldInfo._createRepeatedField(_message!)
..addAll(list);
_values[index] = fieldInfo._createRepeatedField()..addAll(list);
}
}
}
Expand Down
30 changes: 11 additions & 19 deletions protobuf/lib/src/protobuf/generated_message.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,22 @@ abstract class GeneratedMessage {
_FieldSet get _fieldSet => __fieldSet!;

GeneratedMessage() {
__fieldSet = _FieldSet(this, info_, eventPlugin);
if (eventPlugin != null) eventPlugin!.attach(this);
__fieldSet = _FieldSet(this, info_);

// The following two returns confuse dart2js into avoiding inlining the
// constructor *body*. A `@pragma('dart2js:never-inline')` annotation on
// the constructor affects inlining of the generative constructor factory,
// not the constructor body that is called from all the subclasses.
//
// TODO(http://dartbug.com/49475): Remove this when there is an annotation
// that will give the desired result.
return;
return; // ignore: dead_code
}

// Overridden by subclasses.
BuilderInfo get info_;

/// Subclasses can override this getter to be notified of changes
/// to protobuf fields.
EventPlugin? get eventPlugin => null;

/// Creates a deep copy of the fields in this message.
/// (The generated code uses [mergeFromMessage].)
@Deprecated('Using this can add significant size overhead to your binary. '
Expand Down Expand Up @@ -331,19 +336,6 @@ abstract class GeneratedMessage {
/// default value if it is not set.
dynamic getField(int tagNumber) => _fieldSet._getField(tagNumber);

/// Creates List implementing a mutable repeated field.
///
/// Mixins may override this method to change the List type. To ensure
/// that the protobuf can be encoded correctly, the returned List must
/// validate all items added to it. This can most easily be done
/// using the [FieldInfo.check] function.
List<T> createRepeatedField<T>(int tagNumber, FieldInfo<T> fi) =>
PbList<T>(check: fi.check!);

/// Creates a Map representing a map field.
Map<K, V> createMapField<K, V>(int tagNumber, MapFieldInfo<K, V> fi) =>
PbMap<K, V>(fi.keyFieldType, fi.valueFieldType);

/// Returns the value of a field, ignoring any defaults.
///
/// For unset or cleared fields, returns null.
Expand Down
4 changes: 2 additions & 2 deletions protobuf/lib/src/protobuf/json.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,10 @@ void _appendJsonList(BuilderInfo meta, _FieldSet fs, List jsonList,
void _appendJsonMap(BuilderInfo meta, _FieldSet fs, List jsonList,
MapFieldInfo fi, ExtensionRegistry? registry) {
final entryMeta = fi.mapEntryBuilderInfo;
final map = fi._ensureMapField(meta, fs) as PbMap<dynamic, dynamic>;
final map = fi._ensureMapField(meta, fs);
for (final jsonEntryDynamic in jsonList) {
final jsonEntry = jsonEntryDynamic as Map<String, dynamic>;
final entryFieldSet = _FieldSet(null, entryMeta, null);
final entryFieldSet = _FieldSet(null, entryMeta);
final convertedKey = _convertJsonValue(
entryMeta,
entryFieldSet,
Expand Down
Loading